From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas De Marchi Subject: Re: [REVIEW][PATCH] Making poll generally useful for sysctls Date: Tue, 27 Mar 2012 01:02:53 -0300 Message-ID: <20120327010253.26e5087f@vader> References: <20120313005855.GA24639@redhat.com> <20120318192755.GB6589@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Eric W. Biederman" , Linux Kernel , linux-fsdevel@vger.kernel.org To: Lucas De Marchi Return-path: Received: from mail-gx0-f174.google.com ([209.85.161.174]:34159 "EHLO mail-gx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751878Ab2C0EDl convert rfc822-to-8bit (ORCPT ); Tue, 27 Mar 2012 00:03:41 -0400 Received: by gghe5 with SMTP id e5so4380389ggh.19 for ; Mon, 26 Mar 2012 21:03:40 -0700 (PDT) In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 26 Mar 2012 14:44:50 -0300 Lucas De Marchi wrote: > Hi Eric, >=20 > On Sat, Mar 24, 2012 at 4:58 AM, Eric W. Biederman > wrote: > > Lucas De Marchi writes: > > > >>> =C2=A0/* A sysctl table is an array of struct ctl_table: */ > >>> =C2=A0struct ctl_table > >>> =C2=A0{ > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0const char *procname; =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 /* Text ID for /proc/sys, or zero */ > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0void *data; > >>> + =C2=A0 =C2=A0 =C2=A0 atomic_t event; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0int maxlen; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0umode_t mode; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0struct ctl_table *child; =C2=A0 =C2=A0= =C2=A0 =C2=A0/* Deprecated */ > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0proc_handler *proc_handler; =C2=A0 =C2= =A0 /* Callback for text formatting */ > >>> - =C2=A0 =C2=A0 =C2=A0 struct ctl_table_poll *poll; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0void *extra1; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0void *extra2; > >>> =C2=A0}; > >>> @@ -1042,6 +1025,7 @@ struct ctl_table_header > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct rcu= _head rcu; > >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0}; > >>> + =C2=A0 =C2=A0 =C2=A0 wait_queue_head_t wait; > >> > >> If you have a waitqueue per table instead of per entry you will ge= t > >> 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. >=20 > Yeah, indeed. >=20 > > For my money that seemed a nice compromise of code simplicity, and > > generality. =C2=A0We could of course do something much closer to wh= at > > sysfs does and allocate and refcount something similar to your > > ctl_table_poll when we have a ctl_table opened. =C2=A0But that just= looked > > like a pain. >=20 > I don't think we want spurious wakeups in favor of a slightly simpler= code. >=20 >=20 > > > > 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 previou= s > >> patch, your master, 3.3 + my previous patch), so I couldn't test. = On > >> top of your tree: > > > > How odd. =C2=A0It should have applied cleanly to my tree and it app= lies > > with just a two line offset top of Linus's latest with my tree merg= ed > > in. =C2=A0Those two lines of offset coming from the two extra inclu= des > > that came in through the merge. > > > > patch -p1 --dry-run < =C2=A0~/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. >=20 > 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 *he= ad, head->set =3D set; head->parent =3D NULL; head->node =3D node; + init_waitqueue_head(&head->wait); if (node) { struct ctl_table *entry; for (entry =3D table; entry->procname; entry++, node++) { -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html