"Paul E. McKenney" wrote:
>
> On Mon, Oct 31, 2005 at 08:51:19PM -0800, Andrew Morton wrote:
> > Ingo Molnar <[email protected]> wrote:
> > >
> > > @@ -1433,7 +1485,16 @@ send_group_sigqueue(int sig, struct sigq
> > > int ret = 0;
> > >
> > > BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> > > - read_lock(&tasklist_lock);
> > > +
> > > + while(!read_trylock(&tasklist_lock)) {
> > > + if (!p->sighand)
> > > + return -1;
> > > + cpu_relax();
> > > + }
> >
> > This looks kind of ugly and quite unobvious.
> >
> > What's going on there?
>
> This was discussed in the following thread:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=112756875713008&w=2
>
> Looks like its author asked for it to be withdrawn in favor of Roland's
> "[PATCH] Call exit_itimers from do_exit, not __exit_signal" patch:
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=113008567108608&w=2
>
> My guess is that "Roland" is "Roland McGrath", but I cannot find the
> referenced patch. Oleg, any enlightenment?
Yes, it was from Roland McGrath:
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=25f407f0b668f5e4ebd5d13e1fb4306ba6427ead
So I think you can remove ->sighand == NULL re-check and do s/read_trylock/read_lock/.
Posix timers are destroyed from do_exit()->exit_itimers(), after that nobody can send
SI_TIMER to this dying thread group (even if cpu-timer was attached to another process).
send_sigqueue() is different,
> @@ -1385,16 +1407,47 @@ send_sigqueue(int sig, struct sigqueue *
> {
> unsigned long flags;
> int ret = 0;
> + struct sighand_struct *sh = p->sighand;
>
> BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> - read_lock(&tasklist_lock);
> +
> + /*
> + * The rcu based delayed sighand destroy makes it possible to
> + * run this without tasklist lock held. The task struct itself
> + * cannot go away as create_timer did get_task_struct().
> + *
> + * We return -1, when the task is marked exiting, so
> + * posix_timer_event can redirect it to the group leader
> + *
> + */
> + rcu_read_lock();
>
> if (unlikely(p->flags & PF_EXITING)) {
> ret = -1;
> goto out_err;
Is it really needed? You are doing this check again below.
> - spin_lock_irqsave(&p->sighand->siglock, flags);
> + spin_lock_irqsave(&sh->siglock, flags);
But 'sh' can be NULL, no? Yes, you already checked PF_EXITING, so this is
very unlikely, but I think it is still possible in theory. 'sh' was loaded
before reading p->flags, but rcu_read_lock() does not imply memory barrier.
Paul, currently I have no time to read the patch carefully, so probably
this all is my misunderstanding.
> @@ -352,6 +359,7 @@ void __exit_signal(struct task_struct *t
> BUG();
> if (!atomic_read(&sig->count))
> BUG();
> + rcu_read_lock();
> spin_lock(&sighand->siglock);
Why rcu_read_lock() here?
> +static inline int get_task_struct_rcu(struct task_struct *t)
> +{
> + int oldusage;
> +
> + do {
> + oldusage = atomic_read(&t->usage);
> + if (oldusage == 0) {
> + return 0;
> + }
> + } while (cmpxchg(&t->usage.counter,
> + oldusage, oldusage + 1) != oldusage);
> + return 1;
> +}
>
I still don't understand the purpose of get_task_struct_rcu().
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
