pagg
[Top] [All Lists]

Re: [patch] Minor PAGG attach/detach semantic change for 2.6.11

To: Kaigai Kohei <kaigai@xxxxxxxxxxxxx>
Subject: Re: [patch] Minor PAGG attach/detach semantic change for 2.6.11
From: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
Date: Wed, 28 Sep 2005 08:02:50 -0700
Cc: Erik Jacobson <erikj@xxxxxxx>, Kingsley Cheung <kingsley@xxxxxxxxxx>, pagg@xxxxxxxxxxx, tonyt@xxxxxxxxxx
In-reply-to: <433A7FE4.5040109@xxxxxxxxxxxxx>
References: <20050617014512.GA10285@xxxxxxxxxx> <20050927201020.GA30433@xxxxxxx> <433A7FE4.5040109@xxxxxxxxxxxxx>
Reply-to: paulmck@xxxxxxxxxx
Sender: pagg-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.1i
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>
> 

<Prev in Thread] Current Thread [Next in Thread>