lkml.org 
[lkml]   [2012]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [REVIEW][PATCH] Making poll generally useful for sysctls
Lucas De Marchi <lucas.demarchi@profusion.mobi> 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.

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.

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.

---
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 21d836f..739615c 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -18,13 +18,15 @@ static const struct inode_operations proc_sys_inode_operations;
static const struct file_operations proc_sys_dir_file_operations;
static 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)
{
- if (!poll)
- return;
+ return (void *)(unsigned long)atomic_read(&table->event);
+}

- atomic_inc(&poll->event);
- wake_up_interruptible(&poll->wait);
+void proc_sys_poll_notify(struct ctl_table_header *head, struct ctl_table *table)
+{
+ atomic_inc(&table->event);
+ wake_up_interruptible(&head->wait);
}

static struct ctl_table root_table[] = {
@@ -171,6 +173,7 @@ static void init_header(struct ctl_table_header *head,
for (entry = table; entry->procname; entry++, node++) {
rb_init_node(&node->node);
node->header = head;
+ atomic_set(&entry->event, 1);
}
}
}
@@ -242,6 +245,8 @@ static void start_unregistering(struct ctl_table_header *p)
/* anything non-NULL; we'll never dereference it */
p->unregistering = ERR_PTR(-EINVAL);
}
+ /* Don't let poll sleep forever on deleted entries */
+ wake_up_interruptible(&p->wait);
/*
* do not remove from the list until nobody holds it; walking the
* list in do_sysctl() relies on that.
@@ -507,6 +512,9 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
error = table->proc_handler(table, write, buf, &res, ppos);
if (!error)
error = res;
+
+ if (write)
+ proc_sys_poll_notify(head, table);
out:
sysctl_head_finish(head);

@@ -534,8 +542,7 @@ static int proc_sys_open(struct inode *inode, struct file *filp)
if (IS_ERR(head))
return PTR_ERR(head);

- if (table->poll)
- filp->private_data = proc_sys_poll_event(table->poll);
+ filp->private_data = proc_sys_poll_event(table);

sysctl_head_finish(head);

@@ -554,21 +561,14 @@ static unsigned int proc_sys_poll(struct file *filp, poll_table *wait)
if (IS_ERR(head))
return POLLERR | POLLHUP;

- if (!table->proc_handler)
- goto out;
-
- if (!table->poll)
- goto out;
-
event = (unsigned long)filp->private_data;
- poll_wait(filp, &table->poll->wait, wait);
+ poll_wait(filp, &head->wait, wait);

- if (event != atomic_read(&table->poll->event)) {
- filp->private_data = proc_sys_poll_event(table->poll);
+ if (event != atomic_read(&table->event)) {
+ filp->private_data = proc_sys_poll_event(table);
ret = POLLIN | POLLRDNORM | POLLERR | POLLPRI;
}

-out:
sysctl_head_finish(head);

return 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_table *, int,
* cover common cases.
*/

-/* Support for userspace poll() to watch for changes */
-struct ctl_table_poll {
- atomic_t event;
- wait_queue_head_t wait;
-};
-
-static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
-{
- return (void *)(unsigned long)atomic_read(&poll->event);
-}
-
-#define __CTL_TABLE_POLL_INITIALIZER(name) { \
- .event = ATOMIC_INIT(0), \
- .wait = __WAIT_QUEUE_HEAD_INITIALIZER(name.wait) }
-
-#define DEFINE_CTL_TABLE_POLL(name) \
- struct ctl_table_poll name = __CTL_TABLE_POLL_INITIALIZER(name)

/* 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;
struct completion *unregistering;
struct ctl_table *ctl_table_arg;
struct ctl_table_root *root;
@@ -1076,7 +1060,7 @@ struct ctl_path {

#ifdef CONFIG_SYSCTL

-void proc_sys_poll_notify(struct ctl_table_poll *poll);
+void proc_sys_poll_notify(struct ctl_table_header *head, struct ctl_table *table);

extern void setup_sysctl_set(struct ctl_table_set *p,
struct ctl_table_root *root,
diff --git a/kernel/utsname_sysctl.c b/kernel/utsname_sysctl.c
index 63da38c..3e91a50 100644
--- a/kernel/utsname_sysctl.c
+++ b/kernel/utsname_sysctl.c
@@ -53,18 +53,12 @@ static int proc_do_uts_string(ctl_table *table, int write,
r = proc_dostring(&uts_table,write,buffer,lenp, ppos);
put_uts(table, write, uts_table.data);

- if (write)
- proc_sys_poll_notify(table->poll);
-
return r;
}
#else
#define proc_do_uts_string NULL
#endif

-static DEFINE_CTL_TABLE_POLL(hostname_poll);
-static DEFINE_CTL_TABLE_POLL(domainname_poll);
-
static struct ctl_table uts_kern_table[] = {
{
.procname = "ostype",
@@ -93,7 +87,6 @@ static struct ctl_table uts_kern_table[] = {
.maxlen = sizeof(init_uts_ns.name.nodename),
.mode = 0644,
.proc_handler = proc_do_uts_string,
- .poll = &hostname_poll,
},
{
.procname = "domainname",
@@ -101,7 +94,6 @@ static struct ctl_table uts_kern_table[] = {
.maxlen = sizeof(init_uts_ns.name.domainname),
.mode = 0644,
.proc_handler = proc_do_uts_string,
- .poll = &domainname_poll,
},
{}
};
@@ -115,6 +107,8 @@ static struct ctl_table uts_root_table[] = {
{}
};

+static struct ctl_table_header *uts_header;
+
#ifdef CONFIG_PROC_SYSCTL
/*
* Notify userspace about a change in a certain entry of uts_kern_table,
@@ -124,13 +118,13 @@ void uts_proc_notify(enum uts_proc proc)
{
struct ctl_table *table = &uts_kern_table[proc];

- proc_sys_poll_notify(table->poll);
+ proc_sys_poll_notify(uts_header, table);
}
#endif

static int __init utsname_sysctl_init(void)
{
- register_sysctl_table(uts_root_table);
+ uts_header = register_sysctl_table(uts_root_table);
return 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2012-03-24 08:57    [from the cache]
©2003-2011 Jasper Spaans