Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Tue, May 16, 2017 at 08:02:17PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jan 11, 2017 at 09:08:57PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> > > On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > > > On Thu, Dec 15, 2016 at 01:46:25PM +0100, Petr Mladek wrote:
> > > > > On Thu 2016-12-08 22:08:59, Luis R. Rodriguez wrote:
> > > > > > On Thu, Dec 08, 2016 at 12:29:42PM -0800, Kees Cook wrote:
> > > > > > > On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> > > > > > > > +       if (atomic_read(&kmod_concurrent) < max_modprobes)
> > > > > > > > +               return 0;
> > > > > > > > +       atomic_dec(&kmod_concurrent);
> > > > > > > > +       return -ENOMEM;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void kmod_umh_threads_put(void)
> > > > > > > > +{
> > > > > > > > +       atomic_dec(&kmod_concurrent);
> > > > > > > > +}
> > > > > > > 
> > > > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > > > atomic_t for reference counting...
> > > > > > 
> > > > > > That's a much broader functional change than I was looking for, but I am up for
> > > > > > it. Can you describe the benefit of using kref you expect or why this is an
> > > > > > ongoing crusade? Since its a larger functional change how about doing this
> > > > > > change later, and we can test impact with the tress test driver. In theory if
> > > > > > there are benefits can't we add a test case to prove the gains?
> > > > > 
> > > > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > > > is working on, see
> > > > > https://lkml.kernel.org/r/20161114174446.832175072@xxxxxxxxxxxxx
> > > > > 
> > > > > The advantage is that the new refcount API handles over and
> > > > > underflow.
> > > > > 
> > > > > Another advantage is that it increments/decrements the value
> > > > > only when it is safe. It uses cmpxchg to make sure that
> > > > > the checks are valid.
> > > > 
> > > > Great thanks, will look into that.
> > > 
> > > OK I've done the conversion now, the only thing is linux-next as of today lacks
> > > KREF_INIT() so I've open coded it for now. Once Peter's changes get merged the
> > > only thing we'dneed is to change the open code line to KREF_INIT().
> > > 
> > > I'll annotate this as Suggested-by Kees and Petr, I did this as a separate atomic
> > > step after this to make it easier for review.
> > 
> > Spoke too soon, kref_read() is not upstream yet either, so I can hold conversion
> > over until Peter's work is merged. Peter please Cc me on those patches if possible
> > :D
> 
> All the needed kref stuff is upstream now, however, kref is overkill for
> kmod_concurrent given this is just a counter, it is not used to release
> any object, and kref_put() requires such mechanism. The lightweight
> refcount_t is much more appropriate here so will use that and respin
> this series, finally.

And... even the refcount_t is overkill here given even with preemption stuff on
inc we still run into the warnings implemented by the recount stuff right away.
The only way to properly fix this is with a proper lock and I don't think this is
worth it at this point.

This would be an issue if the accounting here was for an object but since its
not and its just a loose estimate for a subjective "reasonable threshold" this
is all just overkill.

Lesson: (unless I hear otherwise)

As such I see no real strong motivation for a change here now. Counters, used
without any object references or any real critical stuff is left best with the
old atomic counters.

  Luis