Re: [PATCH] Change task_struct->comm to use RCU.

[Paul E. McKenney]

On Wed, Feb 26, 2014 at 10:44:32PM +0900, Tetsuo Handa wrote:
> Thank you for reviewing, Paul.

No problem, but you do also need to address Lai Jiangshan's and
Peter Zijlstra's questions about the purpose of this patch.  I was
looking at it only from a "does it work?" viewpoint.

> Paul E. McKenney wrote:
> > > +/**
> > > + * set_task_comm - Change task_struct->comm with tracer and perf hooks called.
> > > + *
> > > + * @tsk: Pointer to "struct task_struct".
> > > + * @buf: New comm name.
> > > + *
> > > + * Returns nothing.
> > > + */
> > > +void set_task_comm(struct task_struct *tsk, char *buf)
> > > +{
> > > +	/*
> > > +	 * Question: Do we need to use task_lock()/task_unlock() ?
> > 
> > The have-memory path through do_set_task_comm() does tolerate multiple
> > CPUs concurrently setting the comm of a given task, but the no-memory
> > path does not.  I advise keeping the locking, at least unless/until
> > some workload is having lots of CPUs concurrently changing a given
> > task's comm.  For some good reason, I hasten to add!  ;-)
> > 
> That question meant whether trace_task_rename(tsk, buf) needs to be serialized
> by tsk->alloc_lock. If trace_task_rename(tsk, buf) doesn't need to be
> serialized by tsk->alloc_lock, we can remove task_lock()/task_unlock().

I was attempting ot answer that question.  ;-)

> > Another reason to get rid of the lock is to allow do_set_task_comm()
> > to sleep waiting for memory to become available, but not sure that
> > this is a good approach.
> 
> I used GFP_ATOMIC because I don't know whether there are callers that depend on
> "changing task->comm does not sleep", for until today "changing task->comm did
> not sleep".

OK...

> > > +void do_set_task_comm(struct task_struct *tsk, const char *buf)
> > > +{
> > > +	struct rcu_comm *comm;
> > > +
> > > +	comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
> > > +	if (likely(comm)) {
> > > +		atomic_set(&comm->usage, 1);
> > > +		strncpy(comm->name, buf, TASK_COMM_LEN - 1);
> > > +		smp_wmb(); /* Behave like rcu_assign_pointer(). */
> > > +		comm = xchg(&tsk->rcu_comm, comm);
> > 
> > The value-returning xchg() implies a full memory barrier, so the
> > smp_wmb() is not needed.
> > 
> I see.
> 
> > > +		put_commname(comm);
> > > +	} else {
> > > +		/*
> > > +		 * Memory allocation failed. We have to tolerate loss of
> > > +		 * consistency.
> > > +		 *
> > > +		 * Question 1: Do we want to reserve some amount of static
> > > +		 * "struct rcu_comm" arrays for use upon allocation failures?
> > > +		 *
> > > +		 * Question 2: Do we perfer unchanged comm name over
> > > +		 * overwritten comm name because comm name is copy-on-write ?
> > > +		 */
> > > +		WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
> > > +		rcu_read_lock();
> > > +		do {
> > > +			comm = rcu_dereference(tsk->rcu_comm);
> > > +		} while (!atomic_read(&comm->usage));
> > 
> > So the idea here is to avoid races with someone who might be freeing
> > this same ->rcu_comm structure, right?  If we see a non-zero reference,
> > they might still free it, but that would be OK because we are in an
> > RCU read-side critical section, so it won't actually be freed until
> > we get done.  And our change is being overwritten by someone else's
> > in that case, but that is OK because it could happen anyway.
> > 
> Right.
> 
> > So the loop shouldn't go through many cycles, especially if memory
> > remains low.
> > 
> > If I am correct, might be worth a comment.  Doubly so if I am wrong.  ;-)
> > 
> You are correct.
> 
> What about Q1 and Q2, like below ?

I suggest reviewing the LKML thread that Peter Zijlstra pointed you at
before digging too much further into this.  Especially this one:

	https://lkml.org/lkml/2011/5/18/408

							Thanx, Paul

>   /* Usage is initialized with ATOMIC_INIT(-1). */
>   static struct rcu_comm static_rcu_comm[CONFIG_RESERVED_COMMBUFFER];
> 
>   static void free_commname(struct rcu_head *rcu)
>   {
>   	struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
>   	if (comm < &static_rcu_comm[0] ||
>   	    comm >= &static_rcu_comm[CONFIG_RESERVED_COMMBUFFER])
>   		kmem_cache_free(commname_cachep, comm);
>   	else
>   		atomic_set(&comm->usage, -1);
>   }
> 
>   void do_set_task_comm(struct task_struct *tsk, const char *buf)
>   {
>   	struct rcu_comm *comm;
>   
>   	comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
>   	if (likely(comm))
>   		atomic_set(&comm->usage, 1);
>   	else {
>   		int i;
>   
>   		/* Memory allocation failed. Try static comm name buffer. */
>   		for (i = 0; i < CONFIG_RESERVED_COMMBUFFER; i++) {
>   			comm = &static_rcu_comm[i];
>   			if (atomic_read(&comm->usage) != -1)
>   				continue;
>   			if (atomic_add_return(&comm->usage, 2) == 1)
>   				goto found;
>   		  	put_commname(comm);
>   		  	put_commname(comm);
>   		}
>   		/* No comm name buffer available. Keep current comm name. */
>   		WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
>   		return;
>   	}
>   found:
>   	strncpy(comm->name, buf, TASK_COMM_LEN - 1);
>   	comm = xchg(&tsk->rcu_comm, comm);
>   	put_commname(comm);
>   }
> 
> Since we can preallocate rcu_comm using GFP_KERNEL and return -ENOMEM (e.g.
> prepare_bprm_creds() for execve() case, copy_from_user() for prctl(PR_SET_NAME)
> and comm_write() (i.e. /proc/$pid/comm ) cases), there will be little users of
> static_rcu_comm[] (e.g. kthreadd()).
> 
> 
> > > @@ -1191,6 +1195,11 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> > >  	p = dup_task_struct(current);
> > >  	if (!p)
> > >  		goto fork_out;
> > > +	rcu_read_lock();
> > > +	do {
> > > +		p->rcu_comm = rcu_dereference(current->rcu_comm);
> > > +	} while (!atomic_inc_not_zero(&p->rcu_comm->usage));
> > 
> > OK, loop until we get the sole reference on the comm...  But
> > I suggest an rcu_read_unlock() followed by an rcu_read_lock() as the
> > first thing in the loop.  Just to prevent adding an RCU CPU stall to
> > our woes if we cannot get a reference.
> > 
> I see. I will use
> 
>   p->rcu_comm = NULL;
>   do {
>   	struct rcu_comm *comm;
>   	rcu_read_lock();
>   	comm = rcu_dereference(current->rcu_comm);
>   	if (atomic_inc_not_zero(&comm->usage))
>   		p->rcu_comm = comm;
>   	rcu_read_unlock();
>   } while (!p->rcu_comm);
> 
> in the next version (if we use RCU approach).
> 
> > And here the two tasks (parent and child) share the name until one
> > or the other either changes its name or exits.  OK.
> > 
> Yes, this is copy on write approach.
> 
> Another approach could use heuristic atomicity; change from "char comm[16]"
> to "long comm[16/sizeof(long)]" and read/write using "long". By using "long",
> we can reduce the possibility of getting the mixture of current comm name and
> comm name to update to, for reading/writing of "long" is atomic.
> 
> The horizontal axis of below map is the strlen() of current comm name and the
> vertical axis is the strlen() of comm name to update to. If sizeof(long) == 8,
> 193 out of 256 patterns can be atomically updated without any locks. Moreover,
> since strlen() of at least one of current comm name and new comm name tends to
> be shorter than sizeof(long) (e.g. "bash" -> "ls", "rc.sysinit" -> "cat",
> "bash" -> "avahi-daemon" ), update of comm name will more likely be atomic
> without any locks.
> 
>     | 0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15
>   --+-----------------------------------------------
>    0| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    1| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    2| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    3| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    4| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    5| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    6| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    7| o  o  o  o  o  o  o  o  o  o  o  o  o  o  o  o
>    8| o  o  o  o  o  o  o  o  o  x  x  x  x  x  x  x
>    9| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   10| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   11| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   12| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   13| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   14| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
>   15| o  o  o  o  o  o  o  o  x  x  x  x  x  x  x  x
> 
> > > +	rcu_read_unlock();
> > > 
> > >  	ftrace_graph_init_task(p);
> > >  	get_seccomp_filter(p);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/