Race free attributes in sysfs

Race free attributes in sysfs

[Pierre Ossman]

Hi Greg,

I'm reworking the sysfs stuff in the MMC layer to be a bit more flexible, but
there is one thing that has me baffled; how do you add attributes to an object
in a race free manner when you have a dynamic set of attributes.

I've looked at other parts of the kernel and they all use:

1. Add object.
2. Add attributes.

To me, it seems like there's a window between 1 and 2 where the object is in
/sys but doesn't have the proper attributes. Life for user space gets very
annoying if it has to add artificial delays to avoid this window.

So how do I do this properly? Something like this would, from my point of view,
be the sane method:

1. Add hidden object.
2. Add attributes.
3. Show object.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: Race free attributes in sysfs

[Dmitry Torokhov]

On Sunday 20 May 2007 07:01, Pierre Ossman wrote:
> Hi Greg,
> 
> I'm reworking the sysfs stuff in the MMC layer to be a bit more flexible, but
> there is one thing that has me baffled; how do you add attributes to an object
> in a race free manner when you have a dynamic set of attributes.
> 
> I've looked at other parts of the kernel and they all use:
> 
> 1. Add object.
> 2. Add attributes.
> 
> To me, it seems like there's a window between 1 and 2 where the object is in
> /sys but doesn't have the proper attributes. Life for user space gets very
> annoying if it has to add artificial delays to avoid this window.
> 
> So how do I do this properly? Something like this would, from my point of view,
> be the sane method:
> 
> 1. Add hidden object.
> 2. Add attributes.
> 3. Show object.
> 

1. dev->uevent_suppress = 1;
2. device_register(dev);
3. device_create_file(dev, ...);
4. dev->uevent_suppress = 0;
5. kobject_uevent(&dev->kobj, KOBJ_ADD);

-- 
Dmitry

Re: Race free attributes in sysfs

[Pierre Ossman]

Dmitry Torokhov wrote:
> 1. dev->uevent_suppress = 1;
> 2. device_register(dev);
> 3. device_create_file(dev, ...);
> 4. dev->uevent_suppress = 0;
> 5. kobject_uevent(&dev->kobj, KOBJ_ADD);
>
>   

Simple enough. Thanks :)

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: Race free attributes in sysfs

[Mark Lord]

Dmitry Torokhov wrote:
> On Sunday 20 May 2007 07:01, Pierre Ossman wrote:
>> Hi Greg,
>>
>> I'm reworking the sysfs stuff in the MMC layer to be a bit more flexible, but
>> there is one thing that has me baffled; how do you add attributes to an object
>> in a race free manner when you have a dynamic set of attributes.
>>
>> I've looked at other parts of the kernel and they all use:
>>
>> 1. Add object.
>> 2. Add attributes.
>>
>> To me, it seems like there's a window between 1 and 2 where the object is in
>> /sys but doesn't have the proper attributes. Life for user space gets very
>> annoying if it has to add artificial delays to avoid this window.
>>
>> So how do I do this properly? Something like this would, from my point of view,
>> be the sane method:
>>
>> 1. Add hidden object.
>> 2. Add attributes.
>> 3. Show object.
>>
> 
> 1. dev->uevent_suppress = 1;
> 2. device_register(dev);
> 3. device_create_file(dev, ...);
> 4. dev->uevent_suppress = 0;
> 5. kobject_uevent(&dev->kobj, KOBJ_ADD);

I don't see how that prevents an already-running udevd
from seeing the partially filled in /sys/ entry,
if it were.. say.. already doing a /sys scan,
or even just during initial startup.

Re: Race free attributes in sysfs

[Dmitry Torokhov]

On Tuesday 22 May 2007 17:24, Mark Lord wrote:
> Dmitry Torokhov wrote:
> > 1. dev->uevent_suppress = 1;
> > 2. device_register(dev);
> > 3. device_create_file(dev, ...);
> > 4. dev->uevent_suppress = 0;
> > 5. kobject_uevent(&dev->kobj, KOBJ_ADD);
> 
> I don't see how that prevents an already-running udevd
> from seeing the partially filled in /sys/ entry,
> if it were.. say.. already doing a /sys scan,
> or even just during initial startup.

I tought udev relies on uevents and does not hunt through sysfs on
its own...

-- 
Dmitry

Re: Race free attributes in sysfs

[Greg KH]

On Tue, May 22, 2007 at 10:43:55PM -0400, Dmitry Torokhov wrote:
> On Tuesday 22 May 2007 17:24, Mark Lord wrote:
> > Dmitry Torokhov wrote:
> > > 1. dev->uevent_suppress = 1;
> > > 2. device_register(dev);
> > > 3. device_create_file(dev, ...);
> > > 4. dev->uevent_suppress = 0;
> > > 5. kobject_uevent(&dev->kobj, KOBJ_ADD);
> > 
> > I don't see how that prevents an already-running udevd
> > from seeing the partially filled in /sys/ entry,
> > if it were.. say.. already doing a /sys scan,
> > or even just during initial startup.
> 
> I tought udev relies on uevents and does not hunt through sysfs on
> its own...

Actually, udev can run without sysfs at all these days :)

And yes, it only starts to look for things when it recieves an event, it
does not "scan" sysfs at all.

thanks,

greg k-h

Re: Race free attributes in sysfs

[Mark Lord]

Greg KH wrote:
> 
> And yes, it only starts to look for things when it recieves an event, it
> does not "scan" sysfs at all.

Does it "look for" only that one event, or does it "scan" at that point?

Re: Race free attributes in sysfs

[Greg KH]

On Wed, May 23, 2007 at 09:27:12AM -0400, Mark Lord wrote:
>  Greg KH wrote:
> > And yes, it only starts to look for things when it recieves an event, it
> > does not "scan" sysfs at all.
> 
>  Does it "look for" only that one event, or does it "scan" at that point?

udev will act on that event, and as I mentioned, not read anything from
sysfs at all, unless a custom rule is in the rules file asking it to
read a specific sysfs file in the tree.

So no "scanning" happens unless specificically asked for.

And as mentioned, udev can work just fine without sysfs enabled at all
now, with the exception of some custom rules for some devices.

thanks,

greg k-h

Re: Race free attributes in sysfs

[Bill Davidsen]

Greg KH wrote:
> On Wed, May 23, 2007 at 09:27:12AM -0400, Mark Lord wrote:
>>  Greg KH wrote:
>>> And yes, it only starts to look for things when it recieves an event, it
>>> does not "scan" sysfs at all.
>>  Does it "look for" only that one event, or does it "scan" at that point?
> 
> udev will act on that event, and as I mentioned, not read anything from
> sysfs at all, unless a custom rule is in the rules file asking it to
> read a specific sysfs file in the tree.
> 
> So no "scanning" happens unless specificically asked for.
> 
> And as mentioned, udev can work just fine without sysfs enabled at all
> now, with the exception of some custom rules for some devices.
> 
I think what Mark is asking is about the case where udev gets an event, 
is told to look in sysfs, and while looking encounters a partially 
described device.

Now that the "this won't happen unless..." cases, could someone cover 
this and state that it either can't happen because {reason} or that if 
it does the result will be {description}.

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: Race free attributes in sysfs

[Greg KH]

On Sat, May 26, 2007 at 12:12:02PM -0400, Bill Davidsen wrote:
>  Greg KH wrote:
> > On Wed, May 23, 2007 at 09:27:12AM -0400, Mark Lord wrote:
> >>  Greg KH wrote:
> >>> And yes, it only starts to look for things when it recieves an event, it
> >>> does not "scan" sysfs at all.
> >>  Does it "look for" only that one event, or does it "scan" at that point?
> > udev will act on that event, and as I mentioned, not read anything from
> > sysfs at all, unless a custom rule is in the rules file asking it to
> > read a specific sysfs file in the tree.
> > So no "scanning" happens unless specificically asked for.
> > And as mentioned, udev can work just fine without sysfs enabled at all
> > now, with the exception of some custom rules for some devices.
>  I think what Mark is asking is about the case where udev gets an event, is 
>  told to look in sysfs, and while looking encounters a partially described 
>  device.
> 
>  Now that the "this won't happen unless..." cases, could someone cover this 
>  and state that it either can't happen because {reason} or that if it does 
>  the result will be {description}.

It can happen if the attributes are added after the device is created,
so you need to add your attributes to the default groups before it is
registered with the driver core.

thanks,

greg k-h

Re: Race free attributes in sysfs

[Kay Sievers]

On 5/20/07, Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> I'm reworking the sysfs stuff in the MMC layer to be a bit more flexible, but
> there is one thing that has me baffled; how do you add attributes to an object
> in a race free manner when you have a dynamic set of attributes.
>
> I've looked at other parts of the kernel and they all use:
>
> 1. Add object.
> 2. Add attributes.
>
> To me, it seems like there's a window between 1 and 2 where the object is in
> /sys but doesn't have the proper attributes. Life for user space gets very
> annoying if it has to add artificial delays to avoid this window.
>
> So how do I do this properly? Something like this would, from my point of view,
> be the sane method:
>
> 1. Add hidden object.
> 2. Add attributes.
> 3. Show object.

Do you have a fixed set of attribute names, where you just want to
create a subset from that matches the individual device, or do you
need some sort of free naming for the attributes?

Kay

Re: Race free attributes in sysfs

[Pierre Ossman]

Kay Sievers wrote:
>
> Do you have a fixed set of attribute names, where you just want to
> create a subset from that matches the individual device, or do you
> need some sort of free naming for the attributes?
>

Currently, the names are fixed (and I don't see any pressing need for
having dynamic names). I do want to have the ability to add attributes
in different parts of the code though since it allows me to group the
sysfs stuff closer to the function it accesses.

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: Race free attributes in sysfs

[Kay Sievers]

On Mon, 2007-05-21 at 20:43 +0200, Pierre Ossman wrote:
> Kay Sievers wrote:
> >
> > Do you have a fixed set of attribute names, where you just want to
> > create a subset from that matches the individual device, or do you
> > need some sort of free naming for the attributes?
> 
> Currently, the names are fixed (and I don't see any pressing need for
> having dynamic names).

We could change the driver-core to suppress the creation of an attribute
if the attribute's show() or store() method returns something like
-ENOENT at registration time?
The driver would pass _all_ possible attributes of the device at
registration time, but the core would only create the attributes which
are implemented for this particular device? Would that work for you?

There are already subsystems who need to do similar things internally
(firewire), and it may be nice to add such functionality to the core.

> I do want to have the ability to add attributes
> in different parts of the code though since it allows me to group the
> sysfs stuff closer to the function it accesses.

You can assign any number of attribute groups to the device. If they
don't have a group name, they will all be created directly at the device
level. Would that work for you?

Thanks,
Kay

Re: Race free attributes in sysfs

[Cornelia Huck]

On Mon, 21 May 2007 21:28:15 +0200,
Kay Sievers <kay.sievers@vrfy.org> wrote:

> We could change the driver-core to suppress the creation of an attribute
> if the attribute's show() or store() method returns something like
> -ENOENT at registration time?
> The driver would pass _all_ possible attributes of the device at
> registration time, but the core would only create the attributes which
> are implemented for this particular device? Would that work for you?
> 
> There are already subsystems who need to do similar things internally
> (firewire), and it may be nice to add such functionality to the core.

This sounds a bit hackish (overloading the meaning of the show() and
store() methods).

> You can assign any number of attribute groups to the device. If they
> don't have a group name, they will all be created directly at the device
> level. Would that work for you?

What about generic "conditional attribute groups"? Add a check() method
which is called just before adding them, and only add them if check()
returned 0 (or doesn't exist)?

Re: Race free attributes in sysfs

[Greg KH]

On Tue, May 22, 2007 at 10:38:57AM +0200, Cornelia Huck wrote:
> On Mon, 21 May 2007 21:28:15 +0200,
> Kay Sievers <kay.sievers@vrfy.org> wrote:
> 
> > We could change the driver-core to suppress the creation of an attribute
> > if the attribute's show() or store() method returns something like
> > -ENOENT at registration time?
> > The driver would pass _all_ possible attributes of the device at
> > registration time, but the core would only create the attributes which
> > are implemented for this particular device? Would that work for you?
> > 
> > There are already subsystems who need to do similar things internally
> > (firewire), and it may be nice to add such functionality to the core.
> 
> This sounds a bit hackish (overloading the meaning of the show() and
> store() methods).

Firewire already does this today, it's actually really nice :)

> > You can assign any number of attribute groups to the device. If they
> > don't have a group name, they will all be created directly at the device
> > level. Would that work for you?
> 
> What about generic "conditional attribute groups"? Add a check() method
> which is called just before adding them, and only add them if check()
> returned 0 (or doesn't exist)?

People want this on a per-attribute basis, if you did it on a group
level, we would have a bunch of groups with only one attribute in it,
which would be messy.

thanks,

greg k-h

Re: Race free attributes in sysfs

[Pierre Ossman]

Kay Sievers wrote:
> We could change the driver-core to suppress the creation of an attribute
> if the attribute's show() or store() method returns something like
> -ENOENT at registration time?
> The driver would pass _all_ possible attributes of the device at
> registration time, but the core would only create the attributes which
> are implemented for this particular device? Would that work for you?
>   

Not sure. Not in an obvious way at least.

It also doesn't feel like "the kernel way". Generally you can
create/allocate an object, assign attributes to it, then activate it.
Couldn't it be done so that I can add sysfs stuff to a device after I
just initialized it? (but before I add it).

> You can assign any number of attribute groups to the device. If they
> don't have a group name, they will all be created directly at the device
> level. Would that work for you?
>
>   

I've had a look at sysfs groups and the biggest beef I have with those
is that they're too low level. In order to use them I first need to
create device attributes, then create an array of pointers to each attr
member. It would be nice if I could just feed an array of device
attributes (i.e. I want wrappers).

Rgds

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

Re: Race free attributes in sysfs

[Kristian Høgsberg]

On 5/22/07, Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> Kay Sievers wrote:
> > We could change the driver-core to suppress the creation of an attribute
> > if the attribute's show() or store() method returns something like
> > -ENOENT at registration time?
> > The driver would pass _all_ possible attributes of the device at
> > registration time, but the core would only create the attributes which
> > are implemented for this particular device? Would that work for you?
> >
>
> Not sure. Not in an obvious way at least.
>
> It also doesn't feel like "the kernel way". Generally you can
> create/allocate an object, assign attributes to it, then activate it.
> Couldn't it be done so that I can add sysfs stuff to a device after I
> just initialized it? (but before I add it).

I agree here - if it was just possible to do this between create and
add, this discussion would be moot.

> > You can assign any number of attribute groups to the device. If they
> > don't have a group name, they will all be created directly at the device
> > level. Would that work for you?
>
> I've had a look at sysfs groups and the biggest beef I have with those
> is that they're too low level. In order to use them I first need to
> create device attributes, then create an array of pointers to each attr
> member. It would be nice if I could just feed an array of device
> attributes (i.e. I want wrappers).

I did a little helper struct for the firewire subsystem that might be
useful on a more general level.  It's struct fw_attribute_group in
drivers/firewire/fw-device.h and the implementation is
init_fw_attribute_group in drivers/firewire/fw-device.c.  But I agree,
attribute groups require a fair bit of boiler plate code.

cheers,
Kristian

Re: Race free attributes in sysfs

[Mark Lord]

Pierre Ossman wrote:
> Kay Sievers wrote:
>> We could change the driver-core to suppress the creation of an attribute
>> if the attribute's show() or store() method returns something like
>> -ENOENT at registration time?
>> The driver would pass _all_ possible attributes of the device at
>> registration time, but the core would only create the attributes which
>> are implemented for this particular device? Would that work for you?
>>   
> 
> Not sure. Not in an obvious way at least.
> 
> It also doesn't feel like "the kernel way". Generally you can
> create/allocate an object, assign attributes to it, then activate it.
> Couldn't it be done so that I can add sysfs stuff to a device after I
> just initialized it? (but before I add it).
> 
>> You can assign any number of attribute groups to the device. If they
>> don't have a group name, they will all be created directly at the device
>> level. Would that work for you?
>>
> I've had a look at sysfs groups and the biggest beef I have with those
> is that they're too low level. In order to use them I first need to
> create device attributes, then create an array of pointers to each attr
> member. It would be nice if I could just feed an array of device
> attributes (i.e. I want wrappers).

And how does this help to avoid the race condition?
Just curious.

Re: Race free attributes in sysfs

[Greg KH]

On Tue, May 22, 2007 at 05:40:50PM +0200, Pierre Ossman wrote:
> Kay Sievers wrote:
> > We could change the driver-core to suppress the creation of an attribute
> > if the attribute's show() or store() method returns something like
> > -ENOENT at registration time?
> > The driver would pass _all_ possible attributes of the device at
> > registration time, but the core would only create the attributes which
> > are implemented for this particular device? Would that work for you?
> >   
> 
> Not sure. Not in an obvious way at least.
> 
> It also doesn't feel like "the kernel way". Generally you can
> create/allocate an object, assign attributes to it, then activate it.
> Couldn't it be done so that I can add sysfs stuff to a device after I
> just initialized it? (but before I add it).

No, unfortunatly it doesn't work that way today, sorry.

> > You can assign any number of attribute groups to the device. If they
> > don't have a group name, they will all be created directly at the device
> > level. Would that work for you?
> >
> >   
> 
> I've had a look at sysfs groups and the biggest beef I have with those
> is that they're too low level. In order to use them I first need to
> create device attributes, then create an array of pointers to each attr
> member. It would be nice if I could just feed an array of device
> attributes (i.e. I want wrappers).

If you can come up with a wrapper that will work, please let me know and
I'll be glad to add it.  Right now, it's pretty darn simple to do this
(look at the USB and PCI core code for examples if you need it.)

Anyway, groups are what you want and need, please use them and then you
don't have to worry about triggering an event later after you have
created your files on your own.

I'll try to get the time to add the "create a group and test the files
before adding them" variant soon so that firewire and others can use
them easier instead of having to roll their own code for it.

thanks,

greg k-h

Re: Race free attributes in sysfs

[Pierre Ossman]

Greg KH wrote:
> If you can come up with a wrapper that will work, please let me know and
> I'll be glad to add it.  Right now, it's pretty darn simple to do this
> (look at the USB and PCI core code for examples if you need it.)
>
>   

I guess we have different views on "simple" :)

I had a look at the usb code, which is why I think the current method is
a bit indirect. Right now I instead copied the function that adds the
device attributes given in the bus_type structure. Couldn't that be used
in a more general manner to get device attribute groups?

> Anyway, groups are what you want and need, please use them and then you
> don't have to worry about triggering an event later after you have
> created your files on your own.
>   

I take it I can supply an array of groups somewhere?

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org