Re: [take16 1/4] kevent: Core files.

[Chase Venters <chase.venters@clientec.com>]

On Wed, 6 Sep 2006, Evgeniy Polyakov wrote:

> > > +
> > > +struct kevent_user
> > > +{
> >
> > These structure names get a little dicey (kevent, kevent_user, ukevent,
> > mukevent)... might there be slightly different names that could be
> > selected to better distinguish the purpose of each?
>
> Like what?
> ukevent means userspace_kevent, but ukevent is much smaller.
> mukevent is mapped userspace kevent, mukevent is again much smaller.

Hmm, well, kevent_user and ukevent are perhaps the only ones I'm concerned 
about. What about calling kevent_user a kevent_queue, kevent_fd or 
kevent_set?

> I decided to use queue length for mmaped buffer, using size of the
> mmapped buffer as queue length is possible too.
> But in any case it is very broken behaviour to introduce any kind of
> overflow and special marking for that - rt signals already have it, no
> need to create additional headache.

Hmm. The concern here is pinned memory, is it not? I'm trying to think of 
the best way to avoid compile-time limits. select() has a rather 
(infamous) compile-time limit of 1,024 thanks to libc (and thanks to the 
bit vector, a glass ceiling). Now, you'd be a fool to use select() on that many
fd's in modern code meant to run on modern UNIXes. But kevent is a new 
system, the grand unified event loop all of us userspace programmers have 
been begging for since many years ago. Glass ceilings tend to hurt when 
you run into them :)

Using the size of the memory mapped buffer as queue length sounds like a 
sane simplification.

> > > +static int kevent_user_ring_init(struct kevent_user *u)
> > > +{
> > > +	int pnum;
> > > +
> > > +	pnum = ALIGN(KEVENT_MAX_EVENTS*sizeof(struct mukevent) +
> > > sizeof(unsigned int), PAGE_SIZE)/PAGE_SIZE;
> >
> > This calculation works with the current constants, but it comes up a page
> > short if, say, KEVENT_MAX_EVENTS were 4095. It also looks incorrect
> > visually since the 'sizeof(unsigned int)' is only factored in once (rather
> > than once per page). I suggest a static / inline __max_kevent_pages()
> > function that either does:
> >
> > return KEVENT_MAX_EVENTS / KEVENTS_ON_PAGE + 1;
> >
> > or
> >
> > int pnum = KEVENT_MAX_EVENTS / KEVENTS_ON_PAGE;
> > if (KEVENT_MAX_EVENTS % KEVENTS_ON_PAGE)
> > 	pnum++;
> > return pnum;
> >
> > Both should be optimized away by the compiler and will give correct
> > answers regardless of the constant values.
>
> Above pnum calculation aligns number of mukevents to pages size with
> appropriate check for (unsigned int), although it is not stated in that
> comment (more clear commant can be found around KEVENTS_ON_PAGE).
> You propose esentially the same calcualtion in the seconds case, while
> first one requires additional page in some cases.

You are right about my first suggestion sometimes coming up a page extra. 
What I'm worried about is that the current ALIGN() based calculation comes 
up a page short if KEVENT_MAX_EVENTS is certain values (say 4095). This is 
because the "unsigned int index" is inside kevent_mring for every page, 
though the ALIGN() calculation just factors in room for one of them. In 
these boundary cases (KEVENT_MAX_EVENTS == 4095), your calculation thinks 
it can fit one last mukevent on a page because it didn't factor in room 
for "unsigned int index" at the start of every page; rather just for one 
page. In this case, the modulus should always come up non-zero, giving us 
the extra required page.

> It is unused, but I'm still waiting on comments if we need
> kevent_get_events() at all - some people wanted to completely eliminate
> that function in favour of total mmap domination.

Interesting idea. It would certainly simplify the interface.

> I have no strong opinion on how to behave in this situation.
> kevent can panic, can free cache, can go to infinite loop or screw up
> the hard drive. Everything is (almost) the same.

Obviously it's not a huge deal :)

If kevent is to screw up the hard drive, though, we must put in an 
exception for it to avoid my music directory.

> > Looking pretty good. This is my first pass of comments, and I'll probably
> > have questions that follow, but I'm trying to get a really good picture of
> > what is going on here for documentation purposes.
>
> Thank you, Chase.
> I will definitely get your comments into account and change related
> bits.

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