On Wed, Sep 28, 2005 at 08:35:00PM +0900, Kaigai Kohei wrote:
> Hi,
>
> Erik Jacobson wrote:
> >I fixed this in the RCU version of pnotify I'm working per the lse-tech
> >community discussion - thanks for the reminder the other day (in a
> >non-list email).
> >
> >If the RCU version crashes and burns for some reason and we go back to
> >the non-CUu one, I'll need to make the fix there too. The function now
> >looks like this. I hope this is what you had in mind (untested as of
> >this moment).
>
> In my understanding, any _write_ operations can not be implemented
> without locking, even if we can use RCU.
> (In addition, RCU conscious writing/update style is required.)
You understanding is quite correct.
RCU protects readers from writers. Something else must be used to
coordinate writers, for example:
1. locking
2. only single designated thread allowed to update
3. carefully crafted sequences of atomic instructions
(but only do this is -really- needed!)
Thanx, Paul
> For example, pnotify permits to attach a new pnotify_subscriber object
> to another task. If someone calls pnotify_subscribe() for other task
> which is doing fork(), there is a possibility to break the
> pnotify_subscriber_list of victim task.
>
> Therefore, procedures with updates such __pnotify_fork() should be
> serialized by somethig locking. RCU is so effective for seldom-write/
> frequentrly-read pass, such as SELinux's Access Vector Cache(AVC).
> But it's not omnipotence, and it restricts write methodology.
>
> In the past, I made a proposition of applying RCU for PAGG. But it might
> be inappropriate for pnotify/PAGG as a general framework.
>
>
> I have attention to another respect. The current pnotify implementation
> requires to hold pnotify_event_list_sem before calling pnotify_get_events().
> Threfore, we must repeat read_lock/unlock(&tasklist_lock) on
> do_each_thread()/while_each_thread() loop as follows:
>
> ----------------------------
> read_lock(&tasklist_lock);
> do_each_thread(g, p) {
> get_task_struct(p);
> read_unlock(&tasklist_lock);
>
> down_read(&p->pnotify_subscriber_list_sem);
> subscriber = pnotify_get_subscriber(p, events->name);
> :
> up_read(&p->pnotify_subscriber_list_sem);
> read_lock(&tasklist_lock);
> << checking, p is dead or not ? >>
> } while_each_thread(g, p);
> read_unlock(&tasklist_lock);
> ----------------------------
>
> I'm happy, if pnotify_subscriber_list would be protected by rwlock.
>
> If rwlock is used, we can not implement pnotify_subscribe() with current
> spec. But is it impossible to prepare pnotify_subscribe_atomic() or
> pnotify_subscribe_bind() which associates task_struct with pre-allocated
> pnotify_events object ?
>
> ---- in rwlock world :-) ---
> read_lock(&tasklist_lock);
> do_each_thread(g, p) {
> read_lock(&p->pnotify_subscriber_list_rwlock);
> subscriber = pnotify_get_subscriber(p, events->name);
> :
> read_unlock(&p->pnotify_subscriber_list_rwlock);
> } while_each_thread(g, p);
> read_unlock(&tasklist_lock);
> ----------------------------
>
> Thanks,
>
>
> >/**
> > * __pnotify_fork - Add kernel module subscriber to same subscribers as
> > parent
> > * @to_task: The child task that will inherit the parent's subscribers
> > * @from_task: The parent task
> > *
> > * Used to attach a new task to the same subscribers the parent has in its
> > * subscriber list.
> > *
> > * The "from" argument is the parent task. The "to" argument is the child
> > * task.
> > *
> > * See Documentation/pnotify.txt for details on
> > * how to handle return codes from the attach function pointer.
> > *
> > * Locking: The to_task is currently in-construction, so we don't
> > * need to worry about write-locks. We do need to be sure the parent's
> > * subscriber list, which we copy here, doesn't go away on us. This is
> > * done via RCU.
> > *
> > */
> >int
> >__pnotify_fork(struct task_struct *to_task, struct task_struct *from_task)
> >{
> > struct pnotify_subscriber *from_subscriber;
> > int ret;
> >
> > /* We need to be sure the parent's list we copy from doesn't
> > disappear */
> > rcu_read_lock();
> >
> > list_for_each_entry_rcu(from_subscriber,
> > &from_task->pnotify_subscriber_list, entry) {
> > struct pnotify_subscriber *to_subscriber = NULL;
> >
> > to_subscriber = pnotify_subscribe(to_task,
> > from_subscriber->events);
> > if (!to_subscriber) {
> > ret=-ENOMEM;
> > __pnotify_exit(to_task);
> > rcu_read_unlock();
> > return ret;
> > }
> > ret = to_subscriber->events->fork(to_task, to_subscriber,
> > from_subscriber->data);
> >
> > rcu_read_unlock(); /* no more to do with the parent's data */
>
> rcu_read_unlovk(); should be deployed outside of the
> list_for_each_entry_rcu(){...}.
>
> >
> > if (ret < 0) {
> > /* Propagates to copy_process as a fork failure */
> > /* No __pnotify_exit because there is one in the
> > failure path
> > * for copy_process in fork.c */
> > return ret; /* Fork failure */
> > }
> > else if (ret > 0) {
> > /* Success, but fork function pointer in the
> > pnotify_events structure
> > * doesn't want the kenrel module subscribed */
> > /* Again, this is the in-construction-child so no
> > write lock */
> > pnotify_unsubscribe(to_subscriber);
> > }
> > }
> >
> > return 0; /* success */
> >}
>
> --
> Linux Promotion Center, NEC
> KaiGai Kohei <kaigai@xxxxxxxxxxxxx>
>
|