Re: [GIT PULL] Driver core fix for 4.5-rc4

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sun, 2016-02-14 at 13:33 -0800, Linus Torvalds wrote:
> On Sun, Feb 14, 2016 at 1:21 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > This means that when you pass an object to a caller(in this case,
> > the
> > bus_find_device), you pass it with an incremented refcount on the
> > embedding object, which is what the caller cares about.  What
> > happens
> > to the klist_node is entirely internal to the callee subsystem.  So
> > you
> > never have to worry about the klist_node being freed, because it's
> > embedded in the object the caller holds a reference to and thus
> > can't
> > be freed.
> 
> So in this case I didn't actually look at the caller, my reaction was
> more to the klist code itself - it doesn't seem to use that
> kref_get_unless_zero()" model anywhere else. So the new code just
> looked a bit out-of-place which in turn made me worry.

The klist code actually pre-dates kref_get_unless_zero() ...

The problem the klist code doesn't contemplate is the inputs to some of
its functions could be derived from upper layers with no awareness of
what the state of the klist membership is.  In this case
klist_iter_init_node() assumes the klist_node is actually on a klist,
but if it's passed in from a layer that has no awareness, this might be
untrue.  Looking through all the prototypes, I think this is the only
place where the already on list assumption is incorrect, so I think
this is the only place in all of the klist code where we actually need
a kref_get_unless_zero().

> 
> As long as there's a reference there that means that things can't go
> away, I guess I'm happy.
> 
> > Yes, that looks fine too.  I was basically assuming the compiler
> > would
> > optimise away the double setting of i->i_cur.
> 
> Usually the compiler won't be able to. Things like
> "kref_get_unless_zero()" end up using ordered atomic ops (ie there's
> a
> memory clobber in there), and gcc will do "I had better make sure
> everything written to memory is up-to-date because now we're going
> atomics".
> 
> So even when things are inlined and gcc sees everything and could in
> theory move things around, doing so around atomics and reference
> counts is not going to happen (and would be very much not ok - think
> about the compiler starting to reorder memory accesses around people
> doing things like that, and shudder).

Hm, OK, I'll do better next time.

James