Message-ID: <20171016144903.GA8410@kroah.com>
Date:   Mon, 16 Oct 2017 16:49:03 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Peter Rajnoha <prajnoha@...hat.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kobject: record and send PREV_SEQNUM with uevents

On Mon, Oct 16, 2017 at 04:00:26PM +0200, Peter Rajnoha wrote:
> On 10/16/2017 03:55 PM, Greg KH wrote:
> > On Mon, Oct 16, 2017 at 02:35:44PM +0200, Peter Rajnoha wrote:
> >> Record last uevent seqnum that was used with kobject's uevent and send
> >> it with next uevent as PREV_SEQNUM=<num> variable.
> >>
> >> This enhances the way we are able to track and handle uevents in userspace
> >> if we are trying to catch up with all the uevents that were generated
> >> while we were not listening or the uevents which were not delivered.
> >> This may happen if the userspace listener is not running yet when the
> >> uevent is triggered or there's a period of time when it's not listening
> >> (e.g. because the userspace listener is being restarted while a new
> >> uevent fires).
> >>
> >> A userspace listener can compare the last uevent seqnum it knows about
> >> with the last uevent seqnum the kernel reports through uevents delivered
> >> to userspace, both genuine and synthetic ones (the ones generated by
> >> writing uevent action name to /sys/.../uevent file). Then, if needed,
> >> userspace may execute appropriate actions based on that. We don't need
> >> to reevaluate and recheck all items, instead, we can concentrate only
> >> on items for which we really and actually need to reflect changed state
> >> in kernel while the userspace listener was not listening for uevents.
> >>
> >> Signed-off-by: Peter Rajnoha <prajnoha@...hat.com>
> >> ---
> >>  include/linux/kobject.h |  1 +
> >>  lib/kobject.c           |  1 +
> >>  lib/kobject_uevent.c    | 15 +++++++++++++--
> >>  3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> >> index e0a6205caa71..ee6db28b6186 100644
> >> --- a/include/linux/kobject.h
> >> +++ b/include/linux/kobject.h
> >> @@ -73,6 +73,7 @@ struct kobject {
> >>  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
> >>  	struct delayed_work	release;
> >>  #endif
> >> +	u64			last_uevent_seqnum;
> >>  	unsigned int state_initialized:1;
> >>  	unsigned int state_in_sysfs:1;
> >>  	unsigned int state_add_uevent_sent:1;
> >> diff --git a/lib/kobject.c b/lib/kobject.c
> >> index 763d70a18941..2cc809f10ae7 100644
> >> --- a/lib/kobject.c
> >> +++ b/lib/kobject.c
> >> @@ -190,6 +190,7 @@ static void kobject_init_internal(struct kobject *kobj)
> >>  		return;
> >>  	kref_init(&kobj->kref);
> >>  	INIT_LIST_HEAD(&kobj->entry);
> >> +	kobj->last_uevent_seqnum = 0;
> >>  	kobj->state_in_sysfs = 0;
> >>  	kobj->state_add_uevent_sent = 0;
> >>  	kobj->state_remove_uevent_sent = 0;
> >> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> >> index f237a09a5862..f1cbed06a395 100644
> >> --- a/lib/kobject_uevent.c
> >> +++ b/lib/kobject_uevent.c
> >> @@ -454,8 +454,19 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> >>  	}
> >>  
> >>  	mutex_lock(&uevent_sock_mutex);
> >> -	/* we will send an event, so request a new sequence number */
> >> -	retval = add_uevent_var(env, "SEQNUM=%llu", (unsigned long long)++uevent_seqnum);
> >> +	/*
> >> +	 * We will send an event, so request a new sequence number.
> >> +	 * Also, record the number for next uevent's PREV_SEQNUM.
> >> +	 */
> >> +	kobj->last_uevent_seqnum = ++uevent_seqnum;
> >> +	retval = add_uevent_var(env, "SEQNUM=%llu",
> >> +				(unsigned long long)uevent_seqnum);
> >> +	if (retval) {
> >> +		mutext_unlock(&uevent_sock_mutex);
> >> +		goto exit;
> >> +	}
> >> +	retval = add_uevent_var(env, "PREV_SEQNUM=%llu",
> >> +				(unsigned long long)kobj->last_uevent_seqnum);
> > 
> > I'm confused, why are we storing "last_uevent_seqnum" here at all, if it
> > is only just the current one-1?  What else do you do with that value?
> I'm sorry, I've sent incorrect patch by mistake. This is the correct one:

Ok, this is a bit better (but it's not in a format I could apply it in,
if I wanted to...)

> From 91fa0d0f5c52b959a5ca4cc79d1a4f5970db75e2 Mon Sep 17 00:00:00 2001
> From: Peter Rajnoha <prajnoha@...hat.com>
> Date: Thu, 12 Oct 2017 11:33:48 +0200
> Subject: [PATCH] kobject: record and send PREV_SEQNUM with uevents
> 
> Record last uevent seqnum that was used with kobject's uevent and send
> it with next uevent as PREV_SEQNUM=<num> variable.
> 
> This enhances the way we are able to track and handle uevents in userspace
> if we are trying to catch up with all the uevents that were generated
> while we were not listening or the uevents which were not delivered.
> This may happen if the userspace listener is not running yet when the
> uevent is triggered or there's a period of time when it's not listening
> (e.g. because the userspace listener is being restarted while a new
> uevent fires).
> 
> A userspace listener can compare the last uevent seqnum it knows about
> with the last uevent seqnum the kernel reports through uevents delivered
> to userspace, both genuine and synthetic ones (the ones generated by
> writing uevent action name to /sys/.../uevent file). Then, if needed,
> userspace may execute appropriate actions based on that. We don't need
> to reevaluate and recheck all items, instead, we can concentrate only
> on items for which we really and actually need to reflect changed state
> in kernel while the userspace listener was not listening for uevents.
> 
> Signed-off-by: Peter Rajnoha <prajnoha@...hat.com>

I don't really understand what userspace can do with this.  Why does it
matter what the sequence number was for a specific kobject?  What can
happen because of this?

Do you have any example libudev code changes to show this in action so I
can maybe understand the need?

thanks,

greg k-h

Powered by blists - more mailing lists