Date:	Wed, 6 Sep 2006 09:23:56 -0500 (CDT)
From:	Chase Venters <chase.venters@...entec.com>
To:	Evgeniy Polyakov <johnpol@....mipt.ru>
cc:	Chase Venters <chase.venters@...entec.com>,
	lkml <linux-kernel@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Ulrich Drepper <drepper@...hat.com>,
	Andrew Morton <akpm@...l.org>, netdev <netdev@...r.kernel.org>,
	Zach Brown <zach.brown@...cle.com>,
	Christoph Hellwig <hch@...radead.org>,
	Johann Borck <johann.borck@...sedata.com>
Subject: Re: [take16 1/4] kevent: Core files.

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists