Re: resume from another engine results in loss of singlestep req.

[Oleg Nesterov]

On 08/12, Oleg Nesterov wrote:
>
> On 08/11, Roland McGrath wrote:
> >
> > @@ -1298,14 +1305,14 @@ static void finish_report(struct utrace_
> >  {
> >     bool clean = (report->takers && !report->detaches);
> >
> > -   if (report->action <= UTRACE_REPORT && !utrace->report) {
> > -           spin_lock(&utrace->lock);
> > -           utrace->report = 1;
> > -           set_tsk_thread_flag(task, TIF_NOTIFY_RESUME);
> > -   } else if (report->action == UTRACE_INTERRUPT && !utrace->interrupt) {
> > +   if (report->action == UTRACE_INTERRUPT && !utrace->interrupt) {
> >             spin_lock(&utrace->lock);
> >             utrace->interrupt = 1;
> >             set_tsk_thread_flag(task, TIF_SIGPENDING);
> > +   } else if (report->action <= UTRACE_REPORT && !utrace->report) {
> > +           spin_lock(&utrace->lock);
> > +           utrace->report = 1;
>
> Hmm. I think this is not exacly right. Suppose that
> report->action == UTRACE_INTERRUPT but utrace->interrupt is already set.
> In that case, since UTRACE_INTERRUPT <= UTRACE_REPORT, we set ->report.
>
> Not really bad, but not right anyway.

I'd suggest:

        static void finish_report(struct utrace_report *report,
                                  struct task_struct *task, struct utrace 
*utrace)
        {
                if (report->action <= UTRACE_REPORT) {
                        bool intr = (report->action == UTRACE_INTERRUPT);
                        if (!(intr ? utrace->interrupt : utrace->report)) {
                                spin_lock(&utrace->lock);
                                if (intr) {
                                        utrace->interrupt = true;
                                        set_tsk_thread_flag(task, 
TIF_SIGPENDING);
                                } else {
                                        utrace->report = true;
                                        set_tsk_thread_flag(task, 
TIF_NOTIFY_RESUME);
                                }
                                spin_unlock(&utrace->lock);
                        }
                }

                if (likely(report->takers && !report->detaches))
                        return;

                spin_lock(&utrace->lock);
                utrace_reset(task, utrace, &report->action);
        }

personally, I don't think it makes sense to try to save one spin_lock()
like the current code does. utrace_reset() is unlikely case.

But we can do

        static void finish_report(struct utrace_report *report,
                                  struct task_struct *task, struct utrace 
*utrace)
        {
                bool clean = (report->takers && !report->detaches);

                if (report->action <= UTRACE_REPORT) {
                        bool intr = (report->action == UTRACE_INTERRUPT);
                        if (!(intr ? utrace->interrupt : utrace->report)) {
                                spin_lock(&utrace->lock);
                                if (intr) {
                                        utrace->interrupt = true;
                                        set_tsk_thread_flag(task, 
TIF_SIGPENDING);
                                } else {
                                        utrace->report = true;
                                        set_tsk_thread_flag(task, 
TIF_NOTIFY_RESUME);
                                }
                                if (unlikely(!clean))
                                        goto reset;
                                spin_unlock(&utrace->lock);
                        }
                }

                if (likely(clean))
                        return;

                spin_lock(&utrace->lock);
        reset:
                utrace_reset(task, utrace, &report->action);
        }

Oleg.