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: Mon, 26 Mar 2012 14:44:50 -0300 Message-ID: References: <20120313005855.GA24639@redhat.com> <20120318192755.GB6589@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Linux Kernel , linux-fsdevel@vger.kernel.org To: "Eric W. Biederman" Return-path: Received: from mail-ey0-f174.google.com ([209.85.215.174]:55308 "EHLO mail-ey0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932823Ab2CZRpL convert rfc822-to-8bit (ORCPT ); Mon, 26 Mar 2012 13:45:11 -0400 Received: by eaaq12 with SMTP id q12so1607992eaa.19 for ; Mon, 26 Mar 2012 10:45:10 -0700 (PDT) In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi Eric, On Sat, Mar 24, 2012 at 4:58 AM, Eric W. Biederman wrote: > Lucas De Marchi writes: > >>> =A0/* A sysctl table is an array of struct ctl_table: */ >>> =A0struct ctl_table >>> =A0{ >>> =A0 =A0 =A0 =A0const char *procname; =A0 =A0 =A0 =A0 =A0 /* Text ID= for /proc/sys, or zero */ >>> =A0 =A0 =A0 =A0void *data; >>> + =A0 =A0 =A0 atomic_t event; >>> =A0 =A0 =A0 =A0int maxlen; >>> =A0 =A0 =A0 =A0umode_t mode; >>> =A0 =A0 =A0 =A0struct ctl_table *child; =A0 =A0 =A0 =A0/* Deprecate= d */ >>> =A0 =A0 =A0 =A0proc_handler *proc_handler; =A0 =A0 /* Callback for = text formatting */ >>> - =A0 =A0 =A0 struct ctl_table_poll *poll; >>> =A0 =A0 =A0 =A0void *extra1; >>> =A0 =A0 =A0 =A0void *extra2; >>> =A0}; >>> @@ -1042,6 +1025,7 @@ struct ctl_table_header >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}; >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct rcu_head rcu; >>> =A0 =A0 =A0 =A0}; >>> + =A0 =A0 =A0 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. =A0We 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. =A0But that just look= ed > like a pain. I don't think we want spurious wakeups in favor of a slightly simpler c= ode. > > 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. =A0It 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. =A0Those two lines of offset coming from the two extra includes > that came in through the merge. > > patch -p1 --dry-run < =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. 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(). Lucas De Marchi -- 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