lkml.org 
[lkml]   [2012]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [REVIEW][PATCH] Making poll generally useful for sysctls
Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:

> On Mon, 26 Mar 2012 14:44:50 -0300
> Lucas De Marchi <lucas.demarchi@profusion.mobi> wrote:
>
>> Hi Eric,
>>
>> On Sat, Mar 24, 2012 at 4:58 AM, Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>>
>> > Here is rebased version of the patch just in case that helps.
>>
>> Now I can apply, but I can't boot: we hit a NULL dereference in
>> __wake_up_common(), called by proc_sys_poll_notify(). It seems that
>> you forgot to initialize the waitqueue with
>> __WAIT_QUEUE_HEAD_INITIALIZER().
>
> Trying again I came up with the following simple oneliner on top
> of your patch. With it I can boot successfully and poll any file
> under /proc/sys (I didn't try many, but there's no reason it would not
> work).

Thanks. I feel silly for that pretty obvious oversight.

There is another bug I am seeing in the sysctl poll code. It needs to
be .read that updates filp->private_data to event, and not .poll.
Otherwise we have what should be a level triggered interface acting like
an edge triggered interface.

Any chance I could get you to cook up a patch for that bug?

> The nice part of this patch is that suddenly all sysctl entries can be
> monitored through poll() instead of having to add adhoc code. However
> that spurious wake ups are not very nice. Eric, what if we keep the
> waitqueue inside the entry and initialize it there, just like we did
> for ->event? This would mean iterating through them on unregister
> though.

Iterating through the all of the table entries on unregister is
not a problem, some code paths for namespace support are doing that
already. Putting the wait queue in struct ctl_table is something
we can't do. struct ctl_table can be freed before the final fput
on a file descriptor and fs/select.c will try to remove freed
wait queue heads, which would get us back to where we came in.

What we can do is use struct ctl_node instead. Either bloating struct
ctl_node or adding putting a pointer to struct ctl_table_poll. The
only tricky part is that I don't believe I have any size information
on how many ctl_node entries I have. So that information would have
to be gathered and kept as well.

After having looked at how large wait_queue_head_t I am reluctant
to pay the price for keeping a wait queue for nodes that we are not
polling. So I am thinking allocate in .poll and free in unregister,
but I don't think I am ambitious enough to code that up.

Eric


\
 
 \ /
  Last update: 2012-03-28 03:59    [W:0.082 / U:3.700 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site