Re: [REVIEW][PATCH] Making poll generally useful for sysctls

[Lucas De Marchi]

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:
> > Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> writes:
> >
> >>> Â/* A sysctl table is an array of struct ctl_table: */
> >>> Âstruct ctl_table
> >>> Â{
> >>> Â Â Â Âconst char *procname; Â Â Â Â Â /* Text ID for /proc/sys, or zero */
> >>> Â Â Â Âvoid *data;
> >>> + Â Â Â atomic_t event;
> >>> Â Â Â Âint maxlen;
> >>> Â Â Â Âumode_t mode;
> >>> Â Â Â Âstruct ctl_table *child; Â Â Â Â/* Deprecated */
> >>> Â Â Â Âproc_handler *proc_handler; Â Â /* Callback for text formatting */
> >>> - Â Â Â struct ctl_table_poll *poll;
> >>> Â Â Â Âvoid *extra1;
> >>> Â Â Â Âvoid *extra2;
> >>> Â};
> >>> @@ -1042,6 +1025,7 @@ struct ctl_table_header
> >>> Â Â Â Â Â Â Â Â};
> >>> Â Â Â Â Â Â Â Âstruct rcu_head rcu;
> >>> Â Â Â Â};
> >>> + Â Â Â wait_queue_head_t wait;
> >>
> >> If you have a waitqueue per table instead of per entry you will get
> >> spurious notifications when other entries change. The easiest way to
> >> test this is to poll /proc/sys/kernel/hostname and change your
> >> domainname.
> >
> > You will get spurious wakeups but not spurious notifications to
> > userspace since event is still per entry.
> 
> Yeah, indeed.
> 
> > For my money that seemed a nice compromise of code simplicity, and
> > generality. ÂWe could of course do something much closer to what
> > sysfs does and allocate and refcount something similar to your
> > ctl_table_poll when we have a ctl_table opened. ÂBut that just looked
> > like a pain.
> 
> I don't think we want spurious wakeups in favor of a slightly simpler code.
> 
> 
> >
> > Of course we already have spurious notifications for hostname and
> > domainname when multiple uts namespaces are involved, but that
> > is a different problem.
> >
> >> I couldn't apply this patch to any tree (linus/master + my previous
> >> patch, your master, 3.3 + my previous patch), so I couldn't test. On
> >> top of your tree:
> >
> > How odd. ÂIt should have applied cleanly to my tree and it applies
> > with just a two line offset top of Linus's latest with my tree merged
> > in. ÂThose two lines of offset coming from the two extra includes
> > that came in through the merge.
> >
> > patch -p1 --dry-run < Â~/tmp/sysctl-poll-test.patch
> > patching file fs/proc/proc_sysctl.c
> > Hunk #1 succeeded at 18 (offset 2 lines).
> > Hunk #2 succeeded at 173 (offset 2 lines).
> > Hunk #3 succeeded at 245 (offset 2 lines).
> > Hunk #4 succeeded at 512 (offset 2 lines).
> > Hunk #5 succeeded at 542 (offset 2 lines).
> > Hunk #6 succeeded at 561 (offset 2 lines).
> > patching file include/linux/sysctl.h
> > patching file kernel/utsname_sysctl.c
> >
> >> [lucas@vader kernel]$ git am /tmp/a.patch
> >> Applying: Making poll generally useful for sysctls
> >> error: patch failed: fs/proc/proc_sysctl.c:16
> >> error: fs/proc/proc_sysctl.c: patch does not apply
> >> error: patch failed: include/linux/sysctl.h:992
> >> error: include/linux/sysctl.h: patch does not apply
> >> Patch failed at 0001 Making poll generally useful for sysctls
> >
> > 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).

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.

Lucas De Marchi

--------------------------

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 739615c..85ae957 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -168,6 +168,7 @@ static void init_header(struct ctl_table_header *head,
 	head->set = set;
 	head->parent = NULL;
 	head->node = node;
+	init_waitqueue_head(&head->wait);
 	if (node) {
 		struct ctl_table *entry;
 		for (entry = table; entry->procname; entry++, node++) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/