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

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 <[email protected]>
>