| Message ID | 20210602133040.398289363@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sched: Cleanup task_struct::state | expand |
----- On Jun 2, 2021, at 9:12 AM, Peter Zijlstra peterz@infradead.org wrote: > When ran from the sched-out path (preempt_notifier or perf_event), > p->state is irrelevant to determine preemption. You can get preempted > with !task_is_running() just fine. > > The right indicator for preemption is if the task is still on the > runqueue in the sched-out path. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > kernel/events/core.c | 7 +++---- > virt/kvm/kvm_main.c | 2 +- > 2 files changed, 4 insertions(+), 5 deletions(-) > > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas > }, > }; > > - if (!sched_in && task->state == TASK_RUNNING) > + if (!sched_in && current->on_rq) { This changes from checking task->state to current->on_rq, but this change from "task" to "current" is not described in the commit message, which is odd. Are we really sure that task == current here ? Thanks, Mathieu > switch_event.event_id.header.misc |= > PERF_RECORD_MISC_SWITCH_OUT_PREEMPT; > + } > > - perf_iterate_sb(perf_event_switch_output, > - &switch_event, > - NULL); > + perf_iterate_sb(perf_event_switch_output, &switch_event, NULL); > } > > /* > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4869,7 +4869,7 @@ static void kvm_sched_out(struct preempt > { > struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn); > > - if (current->state == TASK_RUNNING) { > + if (current->on_rq) { > WRITE_ONCE(vcpu->preempted, true); > WRITE_ONCE(vcpu->ready, true); > }
On Wed, Jun 02, 2021 at 09:59:07AM -0400, Mathieu Desnoyers wrote: > ----- On Jun 2, 2021, at 9:12 AM, Peter Zijlstra peterz@infradead.org wrote: > > > When ran from the sched-out path (preempt_notifier or perf_event), > > p->state is irrelevant to determine preemption. You can get preempted > > with !task_is_running() just fine. > > > > The right indicator for preemption is if the task is still on the > > runqueue in the sched-out path. > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > kernel/events/core.c | 7 +++---- > > virt/kvm/kvm_main.c | 2 +- > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas > > }, > > }; > > > > - if (!sched_in && task->state == TASK_RUNNING) > > + if (!sched_in && current->on_rq) { > > This changes from checking task->state to current->on_rq, but this change > from "task" to "current" is not described in the commit message, which is odd. > > Are we really sure that task == current here ? Yeah, @task == @prev == current at this point, but yes, not sure why I changed that... lemme change that back to task.
----- On Jun 2, 2021, at 9:12 AM, Peter Zijlstra peterz@infradead.org wrote: [...] > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas > }, > }; > > - if (!sched_in && task->state == TASK_RUNNING) > + if (!sched_in && current->on_rq) { > switch_event.event_id.header.misc |= > PERF_RECORD_MISC_SWITCH_OUT_PREEMPT; > + } > > - perf_iterate_sb(perf_event_switch_output, > - &switch_event, > - NULL); > + perf_iterate_sb(perf_event_switch_output, &switch_event, NULL); > } There is a lot of code cleanup going on here which does not seem to belong to a "Fix" patch. Thanks, Mathieu
On Wed, Jun 02, 2021 at 10:15:16AM -0400, Mathieu Desnoyers wrote: > ----- On Jun 2, 2021, at 9:12 AM, Peter Zijlstra peterz@infradead.org wrote: > [...] > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas > > }, > > }; > > > > - if (!sched_in && task->state == TASK_RUNNING) > > + if (!sched_in && current->on_rq) { > > switch_event.event_id.header.misc |= > > PERF_RECORD_MISC_SWITCH_OUT_PREEMPT; > > + } > > > > - perf_iterate_sb(perf_event_switch_output, > > - &switch_event, > > - NULL); > > + perf_iterate_sb(perf_event_switch_output, &switch_event, NULL); > > } > > There is a lot of code cleanup going on here which does not seem to belong > to a "Fix" patch. Maybe, but I so hate whitespace only patches :-/
On Wed, Jun 02, 2021 at 04:10:29PM +0200, Peter Zijlstra wrote: > On Wed, Jun 02, 2021 at 09:59:07AM -0400, Mathieu Desnoyers wrote: > > ----- On Jun 2, 2021, at 9:12 AM, Peter Zijlstra peterz@infradead.org wrote: > > > > > When ran from the sched-out path (preempt_notifier or perf_event), > > > p->state is irrelevant to determine preemption. You can get preempted > > > with !task_is_running() just fine. > > > > > > The right indicator for preemption is if the task is still on the > > > runqueue in the sched-out path. > > > > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > --- > > > kernel/events/core.c | 7 +++---- > > > virt/kvm/kvm_main.c | 2 +- > > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > > > --- a/kernel/events/core.c > > > +++ b/kernel/events/core.c > > > @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas > > > }, > > > }; > > > > > > - if (!sched_in && task->state == TASK_RUNNING) > > > + if (!sched_in && current->on_rq) { > > > > This changes from checking task->state to current->on_rq, but this change > > from "task" to "current" is not described in the commit message, which is odd. > > > > Are we really sure that task == current here ? > > Yeah, @task == @prev == current at this point, but yes, not sure why I > changed that... lemme change that back to task. FWIW, with that: Acked-by: Mark Rutland <mark.rutland@arm.com> I have no strong feelings either way w.r.t. the whitespace cleanup. ;) Thanks, Mark
--- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8568,13 +8568,12 @@ static void perf_event_switch(struct tas }, }; - if (!sched_in && task->state == TASK_RUNNING) + if (!sched_in && current->on_rq) { switch_event.event_id.header.misc |= PERF_RECORD_MISC_SWITCH_OUT_PREEMPT; + } - perf_iterate_sb(perf_event_switch_output, - &switch_event, - NULL); + perf_iterate_sb(perf_event_switch_output, &switch_event, NULL); } /* --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4869,7 +4869,7 @@ static void kvm_sched_out(struct preempt { struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn); - if (current->state == TASK_RUNNING) { + if (current->on_rq) { WRITE_ONCE(vcpu->preempted, true); WRITE_ONCE(vcpu->ready, true); }
When ran from the sched-out path (preempt_notifier or perf_event), p->state is irrelevant to determine preemption. You can get preempted with !task_is_running() just fine. The right indicator for preemption is if the task is still on the runqueue in the sched-out path. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/events/core.c | 7 +++---- virt/kvm/kvm_main.c | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)