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: Sat, 24 Mar 2012 03:20:53 -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: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Fri, Mar 23, 2012 at 9:25 PM, Eric W. Biederman wrote: > > Lucas can you take a look at the patch below. I don't have a test > harness to test this but this should make poll generally useful > for all sysctls as well as fix the module removal case. > > In particular if you could test to see if the code still works as > expected for the hostname and domainname cases that would be great. > > I don't anticipate any problems but real world testing is always good= =2E > > =A0fs/proc/proc_sysctl.c =A0 | =A0 34 +++++++++++++++++--------------= --- > =A0include/linux/sysctl.h =A0| =A0 22 +++------------------- > =A0kernel/utsname_sysctl.c | =A0 14 ++++---------- > =A03 files changed, 24 insertions(+), 46 deletions(-) > --- > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 47b474b..98fd5c9 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -16,13 +16,15 @@ static const struct inode_operations proc_sys_ino= de_operations; > =A0static const struct file_operations proc_sys_dir_file_operations; > =A0static const struct inode_operations proc_sys_dir_operations; > > -void proc_sys_poll_notify(struct ctl_table_poll *poll) > +static inline void *proc_sys_poll_event(struct ctl_table *table) > =A0{ > - =A0 =A0 =A0 if (!poll) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; > + =A0 =A0 =A0 return (void *)(unsigned long)atomic_read(&table->event= ); > +} > > - =A0 =A0 =A0 atomic_inc(&poll->event); > - =A0 =A0 =A0 wake_up_interruptible(&poll->wait); > +void proc_sys_poll_notify(struct ctl_table_header *head, struct ctl_= table *table) > +{ > + =A0 =A0 =A0 atomic_inc(&table->event); > + =A0 =A0 =A0 wake_up_interruptible(&head->wait); > =A0} > > =A0static struct ctl_table root_table[] =3D { > @@ -169,6 +171,7 @@ static void init_header(struct ctl_table_header *= head, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0for (entry =3D table; entry->procname;= entry++, node++) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rb_init_node(&node->no= de); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0node->header =3D head; > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 atomic_set(&entry->even= t, 1); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > =A0 =A0 =A0 =A0} > =A0} > @@ -240,6 +243,8 @@ static void start_unregistering(struct ctl_table_= header *p) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* anything non-NULL; we'll never dere= ference it */ > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0p->unregistering =3D ERR_PTR(-EINVAL); > =A0 =A0 =A0 =A0} > + =A0 =A0 =A0 /* Don't let poll sleep forever on deleted entries */ > + =A0 =A0 =A0 wake_up_interruptible(&p->wait); > =A0 =A0 =A0 =A0/* > =A0 =A0 =A0 =A0 * do not remove from the list until nobody holds it; = walking the > =A0 =A0 =A0 =A0 * list in do_sysctl() relies on that. > @@ -505,6 +510,9 @@ static ssize_t proc_sys_call_handler(struct file = *filp, void __user *buf, > =A0 =A0 =A0 =A0error =3D table->proc_handler(table, write, buf, &res,= ppos); > =A0 =A0 =A0 =A0if (!error) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error =3D res; > + > + =A0 =A0 =A0 if (write) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 proc_sys_poll_notify(head, table); > =A0out: > =A0 =A0 =A0 =A0sysctl_head_finish(head); > > @@ -532,8 +540,7 @@ static int proc_sys_open(struct inode *inode, str= uct file *filp) > =A0 =A0 =A0 =A0if (IS_ERR(head)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return PTR_ERR(head); > > - =A0 =A0 =A0 if (table->poll) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 filp->private_data =3D proc_sys_poll_ev= ent(table->poll); > + =A0 =A0 =A0 filp->private_data =3D proc_sys_poll_event(table); > > =A0 =A0 =A0 =A0sysctl_head_finish(head); > > @@ -552,21 +559,14 @@ static unsigned int proc_sys_poll(struct file *= filp, poll_table *wait) > =A0 =A0 =A0 =A0if (IS_ERR(head)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return POLLERR | POLLHUP; > > - =A0 =A0 =A0 if (!table->proc_handler) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > - > - =A0 =A0 =A0 if (!table->poll) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; > - > =A0 =A0 =A0 =A0event =3D (unsigned long)filp->private_data; > - =A0 =A0 =A0 poll_wait(filp, &table->poll->wait, wait); > + =A0 =A0 =A0 poll_wait(filp, &head->wait, wait); > > - =A0 =A0 =A0 if (event !=3D atomic_read(&table->poll->event)) { > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 filp->private_data =3D proc_sys_poll_ev= ent(table->poll); > + =A0 =A0 =A0 if (event !=3D atomic_read(&table->event)) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 filp->private_data =3D proc_sys_poll_ev= ent(table); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D POLLIN | POLLRDNORM | POLLERR = | POLLPRI; > =A0 =A0 =A0 =A0} > > -out: > =A0 =A0 =A0 =A0sysctl_head_finish(head); > > =A0 =A0 =A0 =A0return ret; > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index c34b4c8..8647ee0 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -992,34 +992,17 @@ extern int proc_do_large_bitmap(struct ctl_tabl= e *, int, > =A0* cover common cases. > =A0*/ > > -/* Support for userspace poll() to watch for changes */ > -struct ctl_table_poll { > - =A0 =A0 =A0 atomic_t event; > - =A0 =A0 =A0 wait_queue_head_t wait; > -}; > - > -static inline void *proc_sys_poll_event(struct ctl_table_poll *poll) > -{ > - =A0 =A0 =A0 return (void *)(unsigned long)atomic_read(&poll->event)= ; > -} > - > -#define __CTL_TABLE_POLL_INITIALIZER(name) { =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 \ > - =A0 =A0 =A0 .event =3D ATOMIC_INIT(0), =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\ > - =A0 =A0 =A0 .wait =3D __WAIT_QUEUE_HEAD_INITIALIZER(name.wait) } > - > -#define DEFINE_CTL_TABLE_POLL(name) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\ > - =A0 =A0 =A0 struct ctl_table_poll name =3D __CTL_TABLE_POLL_INITIAL= IZER(name) > > =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 f= or /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/* Deprecated = */ > =A0 =A0 =A0 =A0proc_handler *proc_handler; =A0 =A0 /* Callback for te= xt 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. 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: [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 Lucas De Marchi