Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> writes: > On Mon, 26 Mar 2012 14:44:50 -0300 > Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> wrote: > >> Hi Eric, >> >> On Sat, Mar 24, 2012 at 4:58 AM, Eric W. Biederman >> <ebiederm@xxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
