Mailing List Archive: Linux: Kernel

[PATCH 0/3] refcounting improvements in sysfs.

Index | Next | Previous | View Threaded

neilb at suse

Mar 23, 2010, 8:20 PM

Post #1 of 9 (386 views)
Permalink

[PATCH 0/3] refcounting improvements in sysfs.

This series tidies up the refcount of sysfs_dirents in sysfs,
using kref where appropriate and a new karef for s_active.
This achieves significant code simplification, especially the first
patch.

This is in part inspired by http://lwn.net/Articles/336224/ :-)

NeilBrown

---

NeilBrown (3):
sysfs: simplify handling for s_active refcount
sysfs: make s_count a kref
kref: create karef and use for sysfs_dirent->s_active

fs/sysfs/dir.c | 74 ++++++++++++++++++++------------------------------
fs/sysfs/mount.c | 3 +-
fs/sysfs/sysfs.h | 19 +++++--------
include/linux/kref.h | 38 ++++++++++++++++++++++++++
lib/kref.c | 13 +++++++++
5 files changed, 90 insertions(+), 57 deletions(-)

--
Signature

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

ebiederm at xmission

Mar 25, 2010, 8:10 PM

Post #2 of 9 (353 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

NeilBrown <neilb [at] suse> writes:

> This series tidies up the refcount of sysfs_dirents in sysfs,
> using kref where appropriate and a new karef for s_active.
> This achieves significant code simplification, especially the first
> patch.
>
> This is in part inspired by http://lwn.net/Articles/336224/ :-)

Neil I would appreciate if you would cc' Tejun and myself on
these kind of patches as we wrote the code and the best canidates
to review it.

Thanks,
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

neilb at suse

Mar 25, 2010, 8:28 PM

Post #3 of 9 (357 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

On Thu, 25 Mar 2010 20:10:15 -0700
ebiederm [at] xmission (Eric W. Biederman) wrote:

> NeilBrown <neilb [at] suse> writes:
>
> > This series tidies up the refcount of sysfs_dirents in sysfs,
> > using kref where appropriate and a new karef for s_active.
> > This achieves significant code simplification, especially the first
> > patch.
> >
> > This is in part inspired by http://lwn.net/Articles/336224/ :-)
>
> Neil I would appreciate if you would cc' Tejun and myself on
> these kind of patches as we wrote the code and the best canidates
> to review it.
>

Yes, you are right of course.
I just pulled names out of MAINTAINERS figuring the maintainer would know
what best to do with the patch.

NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

teheo at suse

Mar 25, 2010, 9:49 PM

Post #4 of 9 (355 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

Hello,

On 03/24/2010 12:20 PM, NeilBrown wrote:
> This series tidies up the refcount of sysfs_dirents in sysfs,
> using kref where appropriate and a new karef for s_active.
> This achieves significant code simplification, especially the first
> patch.
>
> This is in part inspired by http://lwn.net/Articles/336224/ :-)

Nice article. In general, yeap, I agree it would be nice to have a
working reference count abstraction. However, kref along with kobject
is a good example of obscurity by abstraction anti pattern. :-)

kobject API incorrectly suggests that it deals with the last put
problem. There still are large number of code paths which do the
following,

if (!(kob = kobject_get(kobj)))
return;

I believe (or at least hope) the actual problem cases are mostly fixed
now but there still are a lot of misconceptions around how stuff built
on kref/kobject is synchronized and they sometimes lead to race
conditions buried deep under several layers of abstractions and it
becomes very hard to see those race conditions when they are buried
deep.

If you want to kill refcounts w/ bias based off switch, please put it
inside an abstraction which at least synchronizes itself properly.
Open coding w/ bias at least warns you that there is some complex
stuff going on and you need to trade carefully. Putting the switch on
a separate flag - people often forget how bits in a flag field are
synchronized - and the rest of refcount in a nice looking kref bundle
is very likely to lead to subtle race conditions which are *very*
difficult to notice.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

teheo at suse

Mar 25, 2010, 10:10 PM

Post #5 of 9 (353 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

One thing to add.

On 03/26/2010 01:49 PM, Tejun Heo wrote:
> Nice article. In general, yeap, I agree it would be nice to have a
> working reference count abstraction. However, kref along with kobject
> is a good example of obscurity by abstraction anti pattern. :-)

I think one of the reasons why k* hasn't really worked as well as it
was orginally imagined to do is the way we - the kernel programmers -
think and work. We add abstractions when something is functionally
necessary, so in a lot of cases the functional requirements become the
implementation and the communication among us (ie. serves as implied
documentation). The k* stuff detracts from this principle. Those
abstractions are there for the purpose of abstracting and our usual
mindset becomes very susceptible to misinterpretations as no easily
identifiable functional requirements are there - we either end up
imaginging something up or waste time frustrated trying to figure out
why the hell that abstraction is there.

This actualy is a very generic problem. When a LOT of people are
trying to work together sharing a lot of infrastructures, it is very
deterimental to impose certain paradigm upon them. People can easily
agree upon functional necessities but one guy's wildest, most
ambitious paradigmatic vision looks like a complete bull to another
gal.

So, let's keep the abstractions to the just necessary level and
communicate at the functional layer. In this case, mount and sysfs
shares the requirement for a refcount w/ a kill switch. I'm not sure
it warrants common abstraction at this stage but if the *function* can
be wrapped nicely along with lockdep annotations and all, why not?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

neilb at suse

Mar 25, 2010, 11:02 PM

Post #6 of 9 (350 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

On Fri, 26 Mar 2010 13:49:36 +0900
Tejun Heo <teheo [at] suse> wrote:

> Hello,
>
> On 03/24/2010 12:20 PM, NeilBrown wrote:
> > This series tidies up the refcount of sysfs_dirents in sysfs,
> > using kref where appropriate and a new karef for s_active.
> > This achieves significant code simplification, especially the first
> > patch.
> >
> > This is in part inspired by http://lwn.net/Articles/336224/ :-)
>
> Nice article. In general, yeap, I agree it would be nice to have a
> working reference count abstraction. However, kref along with kobject
> is a good example of obscurity by abstraction anti pattern. :-)

I'm not at all sure that opinion would be universal....

refcounting is something that it is quite easy to get wrong. There are
several slightly different models for refcounting and if you don't have a
clear understanding of the different use cases it is easy to get confused
about exactly what model is being used and so use a refcount wrongly.
kref certainly doesn't cover all models for refcounting but it does cover one
fairly common one very well and I think that it's use bring clarity rather
than obscurity.
Of course if it is used for a refcount which should really follow a different
model then that can cause confusion...

>
> kobject API incorrectly suggests that it deals with the last put
> problem. There still are large number of code paths which do the
> following,
>
> if (!(kob = kobject_get(kobj)))
> return;

kobject_get *always* returns exactly the argument that was passed to it.
(kref_get doesn't have a return value.)

I don't see how the code above has any bearing on the last-put problem, which
I think kref and thus kobject do handle exactly correctly.

>
> I believe (or at least hope) the actual problem cases are mostly fixed
> now but there still are a lot of misconceptions around how stuff built
> on kref/kobject is synchronized and they sometimes lead to race
> conditions buried deep under several layers of abstractions and it
> becomes very hard to see those race conditions when they are buried
> deep.

I agree that there probably misconceptions about how kref works and they are
probably based on a lack of appreciation of the subtle differences in
flavours of refcounts. Hence my desire to create and document different
k*ref types which clarify the different use cases.

>
> If you want to kill refcounts w/ bias based off switch, please put it
> inside an abstraction which at least synchronizes itself properly.
> Open coding w/ bias at least warns you that there is some complex
> stuff going on and you need to trade carefully. Putting the switch on
> a separate flag - people often forget how bits in a flag field are
> synchronized - and the rest of refcount in a nice looking kref bundle
> is very likely to lead to subtle race conditions which are *very*
> difficult to notice.

The only other use of a BIAS that I am aware of is in struct super_block, and
Al Viro recently removed that in his bleeding edge tree (two days before I
sent him a patch to do the same thing:-)

It is dangerous to build too much into an abstraction else you will find that
no-one uses it as it is too specific.

The s_count and s_active in struct super_block are very similar to s_count
and s_active in struct sysfs_dirent, however they are also quite different.
super_block uses a non-atomic s_count (because a spinlock is always held
anyway) and has a separate way of preventing new s_active references (s_root
becomes NULL). The only real similarity is that they both have an 'active'
refcount that *can* become zero and still be visible, which is different to a
kref but still a model worth encapsulating (I think) in karef.

BTW I'd be perfectly happy if the first patch was taken and subsequent ones
not. I think they are a good idea, but I'm happy to forgo them (for now:-).

NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

teheo at suse

Mar 25, 2010, 11:32 PM

Post #7 of 9 (347 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

Hello, Neil.

On 03/26/2010 03:02 PM, Neil Brown wrote:
>> Nice article. In general, yeap, I agree it would be nice to have a
>> working reference count abstraction. However, kref along with kobject
>> is a good example of obscurity by abstraction anti pattern. :-)
>
> I'm not at all sure that opinion would be universal....
>
> refcounting is something that it is quite easy to get wrong. There are
> several slightly different models for refcounting and if you don't have a
> clear understanding of the different use cases it is easy to get confused
> about exactly what model is being used and so use a refcount wrongly.
> kref certainly doesn't cover all models for refcounting but it does cover one
> fairly common one very well and I think that it's use bring clarity rather
> than obscurity.
> Of course if it is used for a refcount which should really follow a different
> model then that can cause confusion...

I don't know. After spending some time with k* and device model, I
grew a pretty strong dislike for abstractions without clear functional
necessities. kref being much simpler than kobject, the abuse is much
less severe but there have been kref misuses (in kobject and SCSI
midlayer) which could have been avoided or at least easily located if
they simply had used atomic_t instead of dreaming up some mystical
properties of kref.

>> kobject API incorrectly suggests that it deals with the last put
>> problem. There still are large number of code paths which do the
>> following,
>>
>> if (!(kob = kobject_get(kobj)))
>> return;
>
> kobject_get *always* returns exactly the argument that was passed to it.
> (kref_get doesn't have a return value.)
>
> I don't see how the code above has any bearing on the last-put
> problem, which I think kref and thus kobject do handle exactly
> correctly.

Oh, I should have been more explicit. It's not directly related to
kref but just something that always comes to my mind when thinking
about k* abstractions. The above bogus condition checks used to be
used quite widely. The programmer for some reason believed the last
kobject_put() somehow will magically make future kobject_get()s return
NULL, which of course doesn't make any sense but hey the
implementation is buried kilometers deep, the API and other usages
seem to suggest that and it's easy to imagine something up when you're
tired.

As an another example, please take a look at the kref API.

int kref_put(struct kref *kref, void (*release) (struct kref *kref));

The function takes @release callback but under which context is it
called? If you look at the source code, it's called in-line which
isn't clear from the API at all (why the hell take a callback if
you're gonna call it in-line?) and there have been *several* subtle
bugs which could have been avoided or at least would have been caught
much easier if it were not for that meaningless separate release
callback. It's just too easy to forget about the executing context
when people write and review stuff over function boundaries.

void put_something(something)
{
if (kref_put(&something->kref))
do something which might dead lock;
}

is way easier to avoid bugs and review than

void really_kill_something(struct kref *kref)
{
struct my_something *something = container_of(...);

do something which might dead lock;
}

void put_something(something)
{
kref_put(&someting->kref);
}

This is *way* worse than atomic_t not better and the confusion is
caused exactly by superflous abstraction which leads the users of the
API to imagine some non-existing function of the abstraction and
hinders the flow of review.

>> I believe (or at least hope) the actual problem cases are mostly fixed
>> now but there still are a lot of misconceptions around how stuff built
>> on kref/kobject is synchronized and they sometimes lead to race
>> conditions buried deep under several layers of abstractions and it
>> becomes very hard to see those race conditions when they are buried
>> deep.
>
> I agree that there probably misconceptions about how kref works and they are
> probably based on a lack of appreciation of the subtle differences in
> flavours of refcounts. Hence my desire to create and document different
> k*ref types which clarify the different use cases.

Oh, I'm not objecting to cleaning up how reference counts are done
per-se but *PLEASE* refrain from introducing abstractions for
abstraction's sake. The k* stuff, device model and sysfs already
walked down that road and got burned badly.

> BTW I'd be perfectly happy if the first patch was taken and
> subsequent ones not. I think they are a good idea, but I'm happy to
> forgo them (for now:-).

If it can be done in a way that it doesn't substitute pure logical
complexity with one which involves memory ordering issues, which is
almost always way worse, I have no objection at all.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

neilb at suse

Mar 28, 2010, 10:10 PM

Post #8 of 9 (322 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

On Fri, 26 Mar 2010 15:32:51 +0900
Tejun Heo <teheo [at] suse> wrote:

> Hello, Neil.
>
> On 03/26/2010 03:02 PM, Neil Brown wrote:
> >> Nice article. In general, yeap, I agree it would be nice to have a
> >> working reference count abstraction. However, kref along with kobject
> >> is a good example of obscurity by abstraction anti pattern. :-)
> >
> > I'm not at all sure that opinion would be universal....
> >
> > refcounting is something that it is quite easy to get wrong. There are
> > several slightly different models for refcounting and if you don't have a
> > clear understanding of the different use cases it is easy to get confused
> > about exactly what model is being used and so use a refcount wrongly.
> > kref certainly doesn't cover all models for refcounting but it does cover one
> > fairly common one very well and I think that it's use bring clarity rather
> > than obscurity.
> > Of course if it is used for a refcount which should really follow a different
> > model then that can cause confusion...
>
> I don't know. After spending some time with k* and device model, I
> grew a pretty strong dislike for abstractions without clear functional
> necessities. kref being much simpler than kobject, the abuse is much
> less severe but there have been kref misuses (in kobject and SCSI
> midlayer) which could have been avoided or at least easily located if
> they simply had used atomic_t instead of dreaming up some mystical
> properties of kref.

Hi Tejun,

this strikes me as really valuable experience that it would be great to
share. While I generally like kref and see value in at least some of
kobject I don't for a moment suppose they are perfect. Fixing them requires
a good understanding of the problems they cause. If you have that knowledge,
it would be a great resource for anyone wanting to 'fix' kobject.
Are you interested in writing anything (more) up at all???

>
> >> kobject API incorrectly suggests that it deals with the last put
> >> problem. There still are large number of code paths which do the
> >> following,
> >>
> >> if (!(kob = kobject_get(kobj)))
> >> return;
> >
> > kobject_get *always* returns exactly the argument that was passed to it.
> > (kref_get doesn't have a return value.)
> >
> > I don't see how the code above has any bearing on the last-put
> > problem, which I think kref and thus kobject do handle exactly
> > correctly.
>
> Oh, I should have been more explicit. It's not directly related to
> kref but just something that always comes to my mind when thinking
> about k* abstractions. The above bogus condition checks used to be
> used quite widely. The programmer for some reason believed the last
> kobject_put() somehow will magically make future kobject_get()s return
> NULL, which of course doesn't make any sense but hey the
> implementation is buried kilometers deep, the API and other usages
> seem to suggest that and it's easy to imagine something up when you're
> tired.

This would argue that having a return value from kobject_get violates Rusty's
law that interfaces should be hard to misuse.
We could probably change that - it is only used 19 times in the current
kernel.
It probably doesn't help that Documentation/kobject.txt includes the text:

A successful call to kobject_get() will increment the kobject's reference
counter and return the pointer to the kobject.

which seems to suggest that an unsuccessful call is possible.

>
> As an another example, please take a look at the kref API.
>
> int kref_put(struct kref *kref, void (*release) (struct kref *kref));
>
> The function takes @release callback but under which context is it
> called? If you look at the source code, it's called in-line which
> isn't clear from the API at all (why the hell take a callback if
> you're gonna call it in-line?) and there have been *several* subtle
> bugs which could have been avoided or at least would have been caught
> much easier if it were not for that meaningless separate release
> callback. It's just too easy to forget about the executing context
> when people write and review stuff over function boundaries.
>
> void put_something(something)
> {
> if (kref_put(&something->kref))
> do something which might dead lock;
> }
>
> is way easier to avoid bugs and review than
>
> void really_kill_something(struct kref *kref)
> {
> struct my_something *something = container_of(...);
>
> do something which might dead lock;
> }
>
> void put_something(something)
> {
> kref_put(&someting->kref);
> }
>
> This is *way* worse than atomic_t not better and the confusion is
> caused exactly by superflous abstraction which leads the users of the
> API to imagine some non-existing function of the abstraction and
> hinders the flow of review.

I'm not immediately convinced by this, though maybe I haven't seen enough
examples yet.

The deadlocks that I have come across would not have been any more obvious in
either of the above - they were caused because sysfs_remove deadlocks when
called from inside a sysfs attribute action...

Also, while this re-write is possible for kref uses it isn't really possible
in kobject as the 'final_put' function must be included in the ktype (though
maybe you don't like that either).

What would be really helpful here is a survey of what sorts of things are
actually done in final_put functions so would could create some guidelines
about how to write the release functions.

Thanks,
NeilBrown

>
> >> I believe (or at least hope) the actual problem cases are mostly fixed
> >> now but there still are a lot of misconceptions around how stuff built
> >> on kref/kobject is synchronized and they sometimes lead to race
> >> conditions buried deep under several layers of abstractions and it
> >> becomes very hard to see those race conditions when they are buried
> >> deep.
> >
> > I agree that there probably misconceptions about how kref works and they are
> > probably based on a lack of appreciation of the subtle differences in
> > flavours of refcounts. Hence my desire to create and document different
> > k*ref types which clarify the different use cases.
>
> Oh, I'm not objecting to cleaning up how reference counts are done
> per-se but *PLEASE* refrain from introducing abstractions for
> abstraction's sake. The k* stuff, device model and sysfs already
> walked down that road and got burned badly.
>
> > BTW I'd be perfectly happy if the first patch was taken and
> > subsequent ones not. I think they are a good idea, but I'm happy to
> > forgo them (for now:-).
>
> If it can be done in a way that it doesn't substitute pure logical
> complexity with one which involves memory ordering issues, which is
> almost always way worse, I have no objection at all.
>
> Thanks.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

teheo at suse

Mar 30, 2010, 8:20 PM

Post #9 of 9 (313 views)
Permalink

Re: [PATCH 0/3] refcounting improvements in sysfs. [In reply to]

Hello, Neil.

On 03/29/2010 02:10 PM, Neil Brown wrote:
>> I don't know. After spending some time with k* and device model, I
>> grew a pretty strong dislike for abstractions without clear functional
>> necessities. kref being much simpler than kobject, the abuse is much
>> less severe but there have been kref misuses (in kobject and SCSI
>> midlayer) which could have been avoided or at least easily located if
>> they simply had used atomic_t instead of dreaming up some mystical
>> properties of kref.
>
> this strikes me as really valuable experience that it would be great
> to share. While I generally like kref and see value in at least
> some of kobject I don't for a moment suppose they are perfect.
> Fixing them requires a good understanding of the problems they
> cause. If you have that knowledge, it would be a great resource for
> anyone wanting to 'fix' kobject. Are you interested in writing
> anything (more) up at all???

I'm not sure I have enough to say to write something up about. :-) One
thing I've been hoping to do is to hide the k* abstraction behind
device model abstraction so that device model API users only have to
deal with device model abstractions - devices and drivers. They are
what the driver writers need and have pretty good tangible concrete
concept about. I think my opinions can be compressed into

* Add abstractions which serve concrete functional necessities as
doing so makes communication among peer programmers much easier by
reducing confusion.

* Don't get too attached to software engineering theories. They're
not bad in themselves but often end up labeling only certain small
subset of all the things which need to be considered, with the
adverse side effect of emphasizing those named ones out of
proportion harming overall trade-off balance.

> This would argue that having a return value from kobject_get violates Rusty's
> law that interfaces should be hard to misuse.
> We could probably change that - it is only used 19 times in the current
> kernel.
> It probably doesn't help that Documentation/kobject.txt includes the text:
>
> A successful call to kobject_get() will increment the kobject's reference
> counter and return the pointer to the kobject.
>
> which seems to suggest that an unsuccessful call is possible.

Yeap, the return value is only for convenience and I think it's more
harmful overall, but then again I suppose most people got used to it
now, which is another important factor to consider. Given that there
are not many users of the return value, I agree that removing it would
be better.

>> This is *way* worse than atomic_t not better and the confusion is
>> caused exactly by superflous abstraction which leads the users of the
>> API to imagine some non-existing function of the abstraction and
>> hinders the flow of review.
>
> I'm not immediately convinced by this, though maybe I haven't seen enough
> examples yet.
>
> The deadlocks that I have come across would not have been any more obvious in
> either of the above - they were caused because sysfs_remove deadlocks when
> called from inside a sysfs attribute action...
>
> Also, while this re-write is possible for kref uses it isn't really possible
> in kobject as the 'final_put' function must be included in the ktype (though
> maybe you don't like that either).

Ah, right, I probably was mixing kobject and kref release functions
when talking about the problems I've seen. I still don't like the
kref API and have chosen atomic_t over it in several cases. To me,
the separate ->release seems way too heavy compared to the
functionality the abstraction actually provides and hinders writing
and reviewing more than it clarifies things.

> What would be really helpful here is a survey of what sorts of things are
> actually done in final_put functions so would could create some guidelines
> about how to write the release functions.

For kobject, I'm not sure what should be done other than hiding it as
much as possible behind device model abstractions. For kref, I think
it would be great if it can be made simpler and dumber so that it only
clarifies the intention of the usage instead of forcing separate
->release. If fancier reference helper is needed, capability to defer
release to a separate context and handle module symbol dereference
problem automatically would be nice provided they can be done in clean
manner, but these are just something I've been vaguely hoping for so
they may be absurd to actually implement. :-)

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads