Re: [take8 2/2] kevent: poll/select() notifications. Timer notifications.

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

On Sat, Aug 12, 2006 at 01:38:35AM -0700, Andrew Morton (akpm@osdl.org) wrote:
> On Sat, 12 Aug 2006 12:18:35 +0400
> Evgeniy Polyakov <johnpol@2ka.mipt.ru> wrote:
> 
> > On Fri, Aug 11, 2006 at 08:45:31AM -0700, Andrew Morton (akpm@osdl.org) wrote:
> > > > +static struct lock_class_key kevent_poll_key;
> > > > +
> > > > +void kevent_poll_reinit(struct file *file)
> > > > +{
> > > > +	lockdep_set_class(&file->st.lock, &kevent_poll_key);
> > > > +}
> > > 
> > > Why is this necessary?
> > 
> > Locks for all storages are initialized in the same function, so lockdep thinks 
> > they are the same, so when later one lock is being held in proces
> > context and other in BH or IRQ lockdep screams, so I reinitialize locks
> > after spin_lock_init().
> 
> So why not simply run spin_lock_init() in the kevent_storage_init() caller?

I separated storage initialization into special function, although it is
quite simple, but it allows not to have a lot of duplicated steps in
each origin (inode, socket, file, AIO, network AIO, poll, timer and so
on). If later some members will be changed or added/removed there will
be no need to change them all instead change one function.

> Does kevent_poll_reinit() have any callers?

Only poll nitialization.

> > > > +	st = (struct kevent_storage *)(t+1);
> > > 
> > > It would be cleaner to create
> > > 
> > > 	struct <something> {
> > > 		struct timer_list timer;
> > > 		struct kevent_storage storage;
> > > 	};
> 
> You missed this?

No problem, I will create such structure instead of pointer-based
scheme.
 
> > > > +	
> > > > +	kevent_storage_dequeue(st, k);
> > > > +	
> > > > +	kfree(t);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int kevent_timer_callback(struct kevent *k)
> > > > +{
> > > > +	struct kevent_storage *st = k->st;
> > > > +	struct timer_list *t = st->origin;
> > > > +
> > > > +	if (!t)
> > > > +		return -ENODEV;
> > > > +	
> > > > +	k->event.ret_data[0] = (__u32)jiffies;
> > > 
> > > What does this do?
> > > 
> > > Does it expose jiffies to userspace?
> > > 
> > > It truncates jiffies on 64-bit machines.
> > 
> > It is a hint when timer was stopped.
> 
> What does that mean?  What is it for?

My test code prints it to stdout and I can show a nice graph of how
precise timer is.

> Does it expose jiffies to userspace?

It is a timestamp of fired condition (measured in jiffies), I can not
say if it exposes jiffies to userspace or not.
It is not a requirement to use that field, it can contain zero or any
other number, I placed jiffies there.

> It truncates jiffies on 64-bit machines.

For the purpose of evnt timestamp it is more than enough.

> Please respond to all review comments and questions.

Sorry if something got lost.

> > > > +late_initcall(kevent_init_timer);
> > > 
> > > module_init() would be more typical.  If there was a reason for using
> > > late_initcall(), that reason should be commented.
> > 
> > No, there are no reasons to use late_initcall() in any kevent
> > initialization function, I do not use module_init() since kevent can not
> > be modular. It can be replaced with pure __init function.
> > Should it?
> 
> We use module_init() for non-modular modules all the time.  Try doing
> grep module_init */*.c

Ok, I will use it instead late_initcall().

-- 
	Evgeniy Polyakov