[PATCH] Reduce number of KOBJ_REMOVE events

[PATCH] Reduce number of KOBJ_REMOVE events

[Zdenek Kabelac]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: kobj_remove.patch --]
[-- Type: text/x-patch, Size: 3584 bytes --]

Author: Zdenek Kabelac <zkabelac@redhat.com>
Date:   Fri Jul 22 15:02:17 2011 +0200

Reduce number of KOBJ_REMOVE events

For now udev recieves 3 event for removal of DM logical volumes. (1 for
bdi and 2 for same block kobject). Reason is dm device generates its
own kobject event with approriate env parameter and block layer sends
another KOBJ_REMOVE event on its own unconditionaly for the same
kobject. As for now only the kobject cleanup checks that the REMOVE
event has been already sent and avoids duplicate REMOVE event.

The patch for kobject_uevent_env() which has been testing for duplicate
REMOVE event did not passed into the mainline (yet?):

https://www.redhat.com/archives/dm-devel/2010-March/msg00107.html 

I'm proposing alternative way around to always use kobject cleanup
routine for sending REMOVE event if it was not send by the module - so
it makes the code few lines shorter.

This patch removes single appearencies of kobject_uevent() prior
kobject removal.

Signed-off-by:  Zdenek Kabelac <zkabelac@redhat.com>

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 129b9e2..431f401 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -439,7 +439,6 @@ void blk_integrity_unregister(struct gendisk *disk)
 
 	bi = disk->integrity;
 
-	kobject_uevent(&bi->kobj, KOBJ_REMOVE);
 	kobject_del(&bi->kobj);
 	kobject_put(&bi->kobj);
 	disk->integrity = NULL;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index d935bd8..a50472d 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -521,7 +521,6 @@ int blk_register_queue(struct gendisk *disk)
 
 	ret = elv_register_queue(q);
 	if (ret) {
-		kobject_uevent(&q->kobj, KOBJ_REMOVE);
 		kobject_del(&q->kobj);
 		blk_trace_remove_sysfs(dev);
 		kobject_put(&dev->kobj);
@@ -541,7 +540,6 @@ void blk_unregister_queue(struct gendisk *disk)
 	if (q->request_fn)
 		elv_unregister_queue(q);
 
-	kobject_uevent(&q->kobj, KOBJ_REMOVE);
 	kobject_del(&q->kobj);
 	blk_trace_remove_sysfs(disk_to_dev(disk));
 	kobject_put(&disk_to_dev(disk)->kobj);
diff --git a/block/elevator.c b/block/elevator.c
index b0b38ce..a8d912b 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -892,7 +892,6 @@ EXPORT_SYMBOL(elv_register_queue);
 
 static void __elv_unregister_queue(struct elevator_queue *e)
 {
-	kobject_uevent(&e->kobj, KOBJ_REMOVE);
 	kobject_del(&e->kobj);
 	e->registered = 0;
 }
diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..8c9af0d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1013,7 +1013,6 @@ done:
  ueventattrError:
 	device_remove_file(dev, &uevent_attr);
  attrError:
-	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	kobject_del(&dev->kobj);
  Error:
 	cleanup_device_parent(dev);
@@ -1132,7 +1131,6 @@ void device_del(struct device *dev)
 	 */
 	if (platform_notify_remove)
 		platform_notify_remove(dev);
-	kobject_uevent(&dev->kobj, KOBJ_REMOVE);
 	cleanup_device_parent(dev);
 	kobject_del(&dev->kobj);
 	put_device(parent);
diff --git a/mm/slub.c b/mm/slub.c
index 35f351f..3c77956 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4720,7 +4720,6 @@ static void sysfs_slab_remove(struct kmem_cache *s)
 		 */
 		return;
 
-	kobject_uevent(&s->kobj, KOBJ_REMOVE);
 	kobject_del(&s->kobj);
 	kobject_put(&s->kobj);
 }
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 1bacca4..f20a945 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -153,7 +153,6 @@ static void del_nbp(struct net_bridge_port *p)
 
 	br_multicast_del_port(p);
 
-	kobject_uevent(&p->kobj, KOBJ_REMOVE);
 	kobject_del(&p->kobj);
 
 	br_netpoll_disable(p);

Re: [PATCH] Reduce number of KOBJ_REMOVE events

[Kay Sievers]

On Fri, 2011-07-22 at 16:22 +0200, Zdenek Kabelac wrote:

> For now udev recieves 3 event for removal of DM logical volumes. (1 for
> bdi and 2 for same block kobject). Reason is dm device generates its
> own kobject event with approriate env parameter and block layer sends
> another KOBJ_REMOVE event on its own unconditionaly for the same
> kobject. As for now only the kobject cleanup checks that the REMOVE
> event has been already sent and avoids duplicate REMOVE event.

> The patch for kobject_uevent_env() which has been testing for duplicate
> REMOVE event did not passed into the mainline (yet?):

No, it's wasn't merged. Subsystems should really not send their own
'add' or 'remove' events. These are properties of the driver core.

> I'm proposing alternative way around to always use kobject cleanup
> routine for sending REMOVE event if it was not send by the module - so
> it makes the code few lines shorter.

The events the core creates are only sent out at release() not at del(),
so we would delay 'remove' events when we keep the device pinned but
it's not valid anymore. We can not do that today, we would need to move
the core-created 'remove' events to del().


For device-mapper, I would prefer to add a '.dev_uevent' callback to the
'block' class let this callback check 'struct block_device_operations'
for a possibly specified '.uevent' callback and call it.

Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
code to the needed properties to the 'remove' event, instead of sending
its own, and see the duplication.

Let me know if you need more details.

Thanks,
Kay

Re: [dm-devel] [PATCH] Reduce number of KOBJ_REMOVE events

[Zdenek Kabelac]

Dne 25.7.2011 02:18, Kay Sievers napsal(a):
> On Fri, 2011-07-22 at 16:22 +0200, Zdenek Kabelac wrote:
> 
>> For now udev recieves 3 event for removal of DM logical volumes. (1 for
>> bdi and 2 for same block kobject). Reason is dm device generates its
>> own kobject event with approriate env parameter and block layer sends
>> another KOBJ_REMOVE event on its own unconditionaly for the same
>> kobject. As for now only the kobject cleanup checks that the REMOVE
>> event has been already sent and avoids duplicate REMOVE event.
> 
>> The patch for kobject_uevent_env() which has been testing for duplicate
>> REMOVE event did not passed into the mainline (yet?):
> 
> No, it's wasn't merged. Subsystems should really not send their own
> 'add' or 'remove' events. These are properties of the driver core.
> 
>> I'm proposing alternative way around to always use kobject cleanup
>> routine for sending REMOVE event if it was not send by the module - so
>> it makes the code few lines shorter.
> 
> The events the core creates are only sent out at release() not at del(),
> so we would delay 'remove' events when we keep the device pinned but
> it's not valid anymore. We can not do that today, we would need to move
> the core-created 'remove' events to del().
> 
> 
> For device-mapper, I would prefer to add a '.dev_uevent' callback to the
> 'block' class let this callback check 'struct block_device_operations'
> for a possibly specified '.uevent' callback and call it.
> 
> Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
> code to the needed properties to the 'remove' event, instead of sending
> its own, and see the duplication.

Sounds like complex solution - maybe it would be easier to just register some
environment variable on dm code side - like kobject_add_env() - so it would
take envs from this internal kobject list and after sending uevent it would
implicitly clear this list.

So in dm case  dm-uevent would just register  env(cookie) for KOBJ_REMOVE and
would left kobject_uevent() on block layer ?

Also I'm aware that remove event would be delayed by leaving it on
kobject_cleanup(), but since you mentioned 'del()' as a better place - why not
move this implicit uvent call there - since most kernel driver writers
probably do not want to be bothered with uvents?

Zdenek

Re: [dm-devel] [PATCH] Reduce number of KOBJ_REMOVE events

[Kay Sievers]

On Mon, Jul 25, 2011 at 12:12, Zdenek Kabelac <zkabelac@redhat.com> wrote:
> Dne 25.7.2011 02:18, Kay Sievers napsal(a):
>> On Fri, 2011-07-22 at 16:22 +0200, Zdenek Kabelac wrote:
>>
>>> For now udev recieves 3 event for removal of DM logical volumes. (1 for
>>> bdi and 2 for same block kobject). Reason is dm device generates its
>>> own kobject event with approriate env parameter and block layer sends
>>> another KOBJ_REMOVE event on its own unconditionaly for the same
>>> kobject. As for now only the kobject cleanup checks that the REMOVE
>>> event has been already sent and avoids duplicate REMOVE event.
>>
>>> The patch for kobject_uevent_env() which has been testing for duplicate
>>> REMOVE event did not passed into the mainline (yet?):
>>
>> No, it's wasn't merged. Subsystems should really not send their own
>> 'add' or 'remove' events. These are properties of the driver core.
>>
>>> I'm proposing alternative way around to always use kobject cleanup
>>> routine for sending REMOVE event if it was not send by the module - so
>>> it makes the code few lines shorter.
>>
>> The events the core creates are only sent out at release() not at del(),
>> so we would delay 'remove' events when we keep the device pinned but
>> it's not valid anymore. We can not do that today, we would need to move
>> the core-created 'remove' events to del().
>>
>> For device-mapper, I would prefer to add a '.dev_uevent' callback to the
>> 'block' class let this callback check 'struct block_device_operations'
>> for a possibly specified '.uevent' callback and call it.
>>
>> Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
>> code to the needed properties to the 'remove' event, instead of sending
>> its own, and see the duplication.
>
> Sounds like complex solution

I don't think so, It's clean, ~30 lines long, and technically correct, I expect.

> maybe it would be easier to just register some
> environment variable on dm code side - like kobject_add_env() - so it would
> take envs from this internal kobject list and after sending uevent it would
> implicitly clear this list.

So we would keep allocated per-event-type variables in the kobject, to
send when 'remove' is finally called? The callbacks are just much
simpler , I guess.

> So in dm case  dm-uevent would just register  env(cookie) for KOBJ_REMOVE and
> would left kobject_uevent() on block layer ?
>
> Also I'm aware that remove event would be delayed by leaving it on
> kobject_cleanup(), but since you mentioned 'del()' as a better place - why not
> move this implicit uvent call there.

It's probably not wrong to do that, but I don't remember now why we
added it to release() that time.

> since most kernel driver writers
> probably do not want to be bothered with uvents?

I guess, most drivers use devices not raw kobjects, which send events
on their own, and don't need anything here.

Kay

Re: [dm-devel] [PATCH] Reduce number of KOBJ_REMOVE events

[Zdenek Kabelac]

Dne 25.7.2011 14:17, Kay Sievers napsal(a):
> On Mon, Jul 25, 2011 at 12:12, Zdenek Kabelac <zkabelac@redhat.com> wrote:
>> Dne 25.7.2011 02:18, Kay Sievers napsal(a):
>>> On Fri, 2011-07-22 at 16:22 +0200, Zdenek Kabelac wrote:
>>>
>>>> For now udev recieves 3 event for removal of DM logical volumes. (1 for
>>>> bdi and 2 for same block kobject). Reason is dm device generates its
>>>> own kobject event with approriate env parameter and block layer sends
>>>> another KOBJ_REMOVE event on its own unconditionaly for the same
>>>> kobject. As for now only the kobject cleanup checks that the REMOVE
>>>> event has been already sent and avoids duplicate REMOVE event.
>>>
>>>> The patch for kobject_uevent_env() which has been testing for duplicate
>>>> REMOVE event did not passed into the mainline (yet?):
>>>
>>> No, it's wasn't merged. Subsystems should really not send their own
>>> 'add' or 'remove' events. These are properties of the driver core.
>>>
>>>> I'm proposing alternative way around to always use kobject cleanup
>>>> routine for sending REMOVE event if it was not send by the module - so
>>>> it makes the code few lines shorter.
>>>
>>> The events the core creates are only sent out at release() not at del(),
>>> so we would delay 'remove' events when we keep the device pinned but
>>> it's not valid anymore. We can not do that today, we would need to move
>>> the core-created 'remove' events to del().
>>>
>>> For device-mapper, I would prefer to add a '.dev_uevent' callback to the
>>> 'block' class let this callback check 'struct block_device_operations'
>>> for a possibly specified '.uevent' callback and call it.
>>>
>>> Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
>>> code to the needed properties to the 'remove' event, instead of sending
>>> its own, and see the duplication.
>>
>> Sounds like complex solution
> 
> I don't think so, It's clean, ~30 lines long, and technically correct, I expect.

Well then I've probably not fully understand your idea here - I guess it would
then simpler written by you?


>> maybe it would be easier to just register some
>> environment variable on dm code side - like kobject_add_env() - so it would
>> take envs from this internal kobject list and after sending uevent it would
>> implicitly clear this list.
> 
> So we would keep allocated per-event-type variables in the kobject, to
> send when 'remove' is finally called? The callbacks are just much
> simpler , I guess.

No - nothing so complex - kobject would have the list - and you would be able
to add some env param to this list - the nearest  kobject_uevent() would just
splice those parameters to the env list it wants to send (something like 10
lines of code). The only user would be probably dm so far - and it would check
it wants to send REMOVE - and in this case it would add env to kobject and
would skip kobject_uvent.

On the other hand, it would probably extended kobject struct size without big
use case - so Milan's solution that checks whether REMOVE has been already
sent and skip all future REMOVE events seems by far the simplest here.

I think your proposal also requires struct extension to store callback somewhere ?

>> So in dm case  dm-uevent would just register  env(cookie) for KOBJ_REMOVE and
>> would left kobject_uevent() on block layer ?
>>
>> Also I'm aware that remove event would be delayed by leaving it on
>> kobject_cleanup(), but since you mentioned 'del()' as a better place - why not
>> move this implicit uvent call there.
> 
> It's probably not wrong to do that, but I don't remember now why we
> added it to release() that time.

del() looks like the best natural place here - and safe few lines of code ;)

Zdenek

Re: [dm-devel] [PATCH] Reduce number of KOBJ_REMOVE events

[Kay Sievers]

On Mon, Jul 25, 2011 at 14:54, Zdenek Kabelac <zkabelac@redhat.com> wrote:
> Dne 25.7.2011 14:17, Kay Sievers napsal(a):

>>>> Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
>>>> code to the needed properties to the 'remove' event, instead of sending
>>>> its own, and see the duplication.
>>>
>>> Sounds like complex solution
>>
>> I don't think so, It's clean, ~30 lines long, and technically correct, I expect.

In include/linux/blkdev.h add:
  int (*uevent) (struct gendisk *, struct kobj_uevent_env *env);
to
  struct block_device_operations

In block/genhd.c add:
  .uevent = block_uevent;
to
  struct class block_class
and have the block_uevent() callback check for an existing uevent() in
the block_device_operations, call it when it's specified.

In drivers/md/dm.c add:
  .dm_uevent;
to
  struct block_device_operations dm_blk_dops
and add all the needed variables there, instead of sending out a faked
device 'remove' event.

>>> maybe it would be easier to just register some
>>> environment variable on dm code side - like kobject_add_env() - so it would
>>> take envs from this internal kobject list and after sending uevent it would
>>> implicitly clear this list.
>>
>> So we would keep allocated per-event-type variables in the kobject, to
>> send when 'remove' is finally called? The callbacks are just much
>> simpler , I guess.
>
> No - nothing so complex - kobject would have the list - and you would be able
> to add some env param to this list - the nearest  kobject_uevent() would just
> splice those parameters to the env list it wants to send (something like 10
> lines of code). The only user would be probably dm so far - and it would check
> it wants to send REMOVE - and in this case it would add env to kobject and
> would skip kobject_uvent.

We don't want to encourage anybody to mess around with events that
way. We also don't want to keep track of subsystem specific event
properties in the core kobject. It's just wrong to send 'remove' from
a driver that uses 'struct device'. It should be changed in dm, not
worked-around in the core.

> On the other hand, it would probably extended kobject struct size without big
> use case - so Milan's solution that checks whether REMOVE has been already
> sent and skip all future REMOVE events seems by far the simplest here.

Yeah, I see the logic, but it doesn't make sending additional 'remove'
events any more correct. We don't want to encourage anybody with code
in the core to do this. 'Add' and 'remove' is nothing struct device
user should send on their own.

> I think your proposal also requires struct extension to store callback somewhere ?

Yeah, it's a block subsys callback, a pointer per block driver, not
per device, nothing that really matters, and it's looks generally
useful.

>>> So in dm case  dm-uevent would just register  env(cookie) for KOBJ_REMOVE and
>>> would left kobject_uevent() on block layer ?
>>>
>>> Also I'm aware that remove event would be delayed by leaving it on
>>> kobject_cleanup(), but since you mentioned 'del()' as a better place - why not
>>> move this implicit uvent call there.
>>
>> It's probably not wrong to do that, but I don't remember now why we
>> added it to release() that time.
>
> del() looks like the best natural place here - and safe few lines of code ;)

Yeah, but again, raw kobjects never send any event, not even 'add'.
It's up the caller to take care of that, if these events are needed.
Many things rightfully never send any event.

The code in the core exists only for the un-common use case that the
caller does send 'add' but does not clean-up the objects properly, and
would introduce an asymmetry with that. We don't want to encourage
anything like that.

The code isn't there to allow additional 'remove' events or to leave
the cleanup to the core in general.

Thanks,
Kay

Re: [dm-devel] [PATCH] Reduce number of KOBJ_REMOVE events

[Kay Sievers]

On Mon, 2011-07-25 at 16:22 +0200, Kay Sievers wrote:
> On Mon, Jul 25, 2011 at 14:54, Zdenek Kabelac <zkabelac@redhat.com> wrote:
> > Dne 25.7.2011 14:17, Kay Sievers napsal(a):
> 
> >>>> Then have 'dm_blk_dops' add '.uevent' and let the core call into the dm
> >>>> code to the needed properties to the 'remove' event, instead of sending
> >>>> its own, and see the duplication.
> >>>
> >>> Sounds like complex solution
> >>
> >> I don't think so, It's clean, ~30 lines long, and technically correct, I expect.
> 
> In include/linux/blkdev.h add:
>   int (*uevent) (struct gendisk *, struct kobj_uevent_env *env);
> to
>   struct block_device_operations
> 
> In block/genhd.c add:
>   .uevent = block_uevent;
> to
>   struct class block_class
> and have the block_uevent() callback check for an existing uevent() in
> the block_device_operations, call it when it's specified.
> 
> In drivers/md/dm.c add:
>   .dm_uevent;
> to
>   struct block_device_operations dm_blk_dops
> and add all the needed variables there, instead of sending out a faked
> device 'remove' event.

Here is the code to illustrate, and fix the issue of needing to send any
'remove' for a device from a driver.

Thanks,
Kay


diff --git a/block/genhd.c b/block/genhd.c
index 6024b82..c2e9714 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1097,8 +1097,21 @@ static void disk_release(struct device *dev)
 	free_part_info(&disk->part0);
 	kfree(disk);
 }
+
+static int block_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+
+	if (!disk->fops)
+		return 0;
+	if (disk->fops->uevent)
+		return disk->fops->uevent(disk, env);
+	return 0;
+}
+
 struct class block_class = {
 	.name		= "block",
+	.dev_uevent	= block_uevent,
 };
 
 static char *block_devnode(struct device *dev, mode_t *mode)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 0cf68b4..22e486e 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2682,11 +2682,17 @@ void dm_free_md_mempools(struct dm_md_mempools *pools)
 	kfree(pools);
 }
 
+static int dm_uevent(struct gendisk *disk, struct kobj_uevent_env *env)
+{
+	return add_uevent_var(env, "COOKIE_MONSTER=1");
+}
+
 static const struct block_device_operations dm_blk_dops = {
 	.open = dm_blk_open,
 	.release = dm_blk_close,
 	.ioctl = dm_blk_ioctl,
 	.getgeo = dm_blk_getgeo,
+	.uevent = dm_uevent,
 	.owner = THIS_MODULE
 };
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1a23722..6dbd69e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1300,6 +1300,7 @@ struct block_device_operations {
 						void **, unsigned long *);
 	unsigned int (*check_events) (struct gendisk *disk,
 				      unsigned int clearing);
+	int (*uevent) (struct gendisk *, struct kobj_uevent_env *env);
 	/* ->media_changed() is DEPRECATED, use ->check_events() instead */
 	int (*media_changed) (struct gendisk *);
 	void (*unlock_native_capacity) (struct gendisk *);