On Tue, Jul 14, 2015 at 4:07 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Tue, Jul 07, 2015 at 03:51:48AM -0700, tip-bot for Andy Lutomirski wrote:
>> +     while (true) {
>> +             u32 cached_flags =
>> +                     READ_ONCE(pt_regs_to_thread_info(regs)->flags);
>> +
>> +             if (!(cached_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
>> +                                   _TIF_UPROBE | _TIF_NEED_RESCHED)))
>> +                     break;
>> +
>> +             /* We have work to do. */
>> +             local_irq_enable();
>> +
>> +             if (cached_flags & _TIF_NEED_RESCHED)
>> +                     schedule();
>> +
>> +             if (cached_flags & _TIF_UPROBE)
>> +                     uprobe_notify_resume(regs);
>> +
>> +             /* deal with pending signal delivery */
>> +             if (cached_flags & _TIF_SIGPENDING)
>> +                     do_signal(regs);
>> +
>> +             if (cached_flags & _TIF_NOTIFY_RESUME) {
>> +                     clear_thread_flag(TIF_NOTIFY_RESUME);
>> +                     tracehook_notify_resume(regs);
>> +             }
>> +
>> +             if (cached_flags & _TIF_USER_RETURN_NOTIFY)
>> +                     fire_user_return_notifiers();
>> +
>> +             /* Disable IRQs and retry */
>> +             local_irq_disable();
>> +     }
>
> I dreamed so many times about this loop in C!

So this made me look at it again, and now I'm worried.

There's that "early break", but it doesn't check
_TIF_USER_RETURN_NOTIFY. So if *only* USER_RETURN_NOTIFY is set, we're
screwed.

It migth be that that doesn't happen for some reason, but I'm not
seeing what that reason would be.

The other thing that worries me is that this depends on all the
handler routines to clear the flags (except for
tracehook_notify_resume()). Which they hopefully do. But that means
that just looking at this locally, it's not at all obvious that it
works right.

So wouldn't it be much nicer to do:

        u32 cached_flags = READ_ONCE(pt_regs_to_thread_info(regs)->flags);

        cached_flags &= _TIF_SIGPENDING | _TIF_NOTIFY_RESUME |
_TIF_USER_RETURN_NOTIFY | _TIF_UPROBE | _TIF_NEED_RESCHED;

        if (!cached_flags)
                break;

        atomic_clear_mask(cached_flags, &pt_regs_to_thread_info(regs)->flags);

and then have those bit tests after that?

                   Linus