Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681075 > unrolled thread

Re: [PATCH] kthread: Atomically set completion and perform dequeue in __kthread_parkme

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] kthread: Atomically set completion and perform dequeue  in __kthread_parkme Peter Zijlstra <peterz@infradead.org> - 2017-07-04 18:10 +0200
    Re: [PATCH] kthread: Atomically set completion and perform dequeue in  __kthread_parkme Vikram Mulukutla <markivx@codeaurora.org> - 2017-07-05 19:30 +0200

#1681075 — Re: [PATCH] kthread: Atomically set completion and perform dequeue in __kthread_parkme

On Mon, Jun 26, 2017 at 03:18:03PM -0700, Vikram Mulukutla wrote:
>  kernel/kthread.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 26db528..7ad3354 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -171,9 +171,20 @@ static void __kthread_parkme(struct kthread *self)
>  {
>  	__set_current_state(TASK_PARKED);
>  	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
> +		/*
> +		 * Why the preempt_disable?
> +		 * Hotplug needs to ensure that 'self' is off of the runqueue
> +		 * as well, before scheduling the stopper thread that will
> +		 * migrate tasks off of the runqeue that 'self' was running on.
> +		 * This avoids unnecessary migration work and also ensures that
> +		 * kthread_unpark in the cpu_up path doesn't race with
> +		 * __kthread_parkme.
> +		 */
> +		preempt_disable();
>  		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
>  			complete(&self->parked);
> +		schedule_preempt_disabled();

This is broken. schedule_preempt_disable() doesn't guarantee no
preemptions, just makes it less likely.

> +		preempt_enable();
>  		__set_current_state(TASK_PARKED);
>  	}
>  	clear_bit(KTHREAD_IS_PARKED, &self->flags);

[toc] | [next] | [standalone]

#1681672 — Re: [PATCH] kthread: Atomically set completion and perform dequeue in __kthread_parkme

On 7/4/2017 9:07 AM, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 03:18:03PM -0700, Vikram Mulukutla wrote:
>>   kernel/kthread.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>> index 26db528..7ad3354 100644
>> --- a/kernel/kthread.c
>> +++ b/kernel/kthread.c
>> @@ -171,9 +171,20 @@ static void __kthread_parkme(struct kthread *self)
>>   {
>>   	__set_current_state(TASK_PARKED);
>>   	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
>> +		/*
>> +		 * Why the preempt_disable?
>> +		 * Hotplug needs to ensure that 'self' is off of the runqueue
>> +		 * as well, before scheduling the stopper thread that will
>> +		 * migrate tasks off of the runqeue that 'self' was running on.
>> +		 * This avoids unnecessary migration work and also ensures that
>> +		 * kthread_unpark in the cpu_up path doesn't race with
>> +		 * __kthread_parkme.
>> +		 */
>> +		preempt_disable();
>>   		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
>>   			complete(&self->parked);
>> +		schedule_preempt_disabled();
> 
> This is broken. schedule_preempt_disable() doesn't guarantee no
> preemptions, just makes it less likely.

Right, the API just informs the scheduler that the calling thread
wishes to have preemption disabled when the API returns. I thought
it was going to guarantee no preemption until the thread is actually
off of the runqueue, but I see the window where an interrupt might
preempt. Doh.

Separate from this hotplug problem, would it be entirely moronic to have
the API disable and enable local interrupts across that short window? I
suppose there's no one that needs this sort of thing so.. no?

> 
>> +		preempt_enable();
>>   		__set_current_state(TASK_PARKED);
>>   	}
>>   	clear_bit(KTHREAD_IS_PARKED, &self->flags);

Thanks,
Vikram

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web