On Tue, Jun 29, 2010 at 03:31:31PM +0200, Oleg Nesterov wrote: > On 06/29, Artem Bityutskiy wrote: > > > > On Wed, 2010-06-23 at 21:41 +0200, Oleg Nesterov wrote: > > > On 06/23, Kees Cook wrote: > > > > > > > > @@ -956,7 +957,15 @@ void set_task_comm(struct task_struct *tsk, char *buf) > > > > */ > > > > memset(tsk->comm, 0, TASK_COMM_LEN); > > > > wmb(); > > > > > > Off-topic. I'd wish I could understand this barrier. Since the lockless > > > reader doesn't do rmb() I don't see how this can help. > > > > This wmb() looks wrong to me as well. To achieve what the comment in > > this function says, it should be smp_wmb() and we should have smp_rmb() > > in the reading side, AFAIU. > > > > > OTOH, I don't > > > understand why it is needed, we never change ->comm[TASK_COMM_LEN-1] == '0'. > > > > I think the idea was that readers can see incomplete names, but not > > messed up names, consisting of old and new ones. > > OK, agreed, comm[TASK_COMM_LEN-1] == '0' can't help to avoid the > messed names. > > But nether can help smp_rmb() in the reading (lockless) side? > > Say, > > printk("comm=%s\n", current->comm); > > if we add rmb() before printk, it can't make any difference. The lockless > code should do something like > > get_comm_lockless(char *to, char *comm) > { > while (*comm++ = *to++) > rmb(); > } > > to ensure it sees the result of > > memset(tsk->comm, 0, TASK_COMM_LEN); > wmb(); > strcpy(tsk->comm, buf); > > in the right order. > > otherwise printk("comm=%s\n", current->comm) can read, say, comm[1] > before set_task_comm->memset(), and comm[0] after set_task_comm->strcpy(). > > So, afaics, set_task_comm()->wmb() buys nothing and should be removed. > The last zero char in task_struct->comm[] is always here, at least this > guarantees that strcpy(char *dest, tsk->comm) is always safe. > > (I cc'ed the expert, Paul can correct me) First, all of the implementations that I can see do task_lock(tsk), which should prevent readers from seeing any changes. So I am guessing that you guys want to allow readers to get at ->comm without having to acquire this lock. So if the reader uses no mutual exclusion, then that reader could be preempted/interrupted/NMIed/cache-missed/whatever for quite some time. The task's ->comm field might change once, twice, ten times in the meantime. The reader might then to so far as to get one character from each of the multiple names that happened in the meantime. So, how to fix? Here are some approaches that come to mind off-hand: 1. Use RCU (always my favorite, of course, but in this case...): The task structure adds a char* named ->commp that normally points to ->comm in the same task structure. The updater changes the name as follows: a. Allocate a chunk of memory to hold the new name. b. Copy the desired string into this newly allocated chunk of memory. c. rcu_assign_pointer() ->commp to point to this newly allocated chunk of memory. d. synchronize_rcu(). e. Now all readers are looking at the new name, so copy the new name into ->comm. f. rcu_assign_pointer() ->commp to point to ->comm. g. synchronize_rcu(). h. free up the newly assigned chunk of memory. Then the reader does: rcu_read_lock() p = rcu_dereference(tsk->commp); do_something_with(p); rcu_read_unlock(); This works, but is a bit complex, and adds not one but two grace periods to the updater. Yes, it is possible to optimize the second grace period away in the case of back-to-back updaters, but... 2. Use sequence locks: The updater just does the update under the sequence lock. The reader does the usual: do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq)); Of course, this can starve readers, which can retaliate using something like the following: i = 0; do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3); if (i >= 3) { write_seqlock(&tsk->commseqlock); do_something_with(tsk->comm); write_sequnlock(&tsk->commseqlock); } Readers might also need rcu_read_lock() to keep the task from disappearing out from under them: rcu_read_lock(); tsk = ... ... i = 0; do { seq = read_seqbegin(&tsk->commseqlock); do_something_with(tsk->comm); } while (read_seqretry(&tsk->commseqlock, seq) && ++i < 3); if (i >= 3) { write_seqlock(&tsk->commseqlock); do_something_with(tsk->comm); write_sequnlock(&tsk->commseqlock); } rcu_read_unlock(); But I have to defer to you guys on this one. For example, in the case where tsk==current, I do not believe that rcu_read_lock() is needed. 3. Various forms of reader-writer locks -- none of which seem to me to work very well in this case. 4. Your approach here! ;-) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html