Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH] ipc/mqueue: avoid sleep after wakeup
To:     Hillf Danton <hdanton@sina.com>
Cc:     Davidlohr Bueso <dbueso@suse.de>,
        Matthias von Faber <matthias.vonfaber@aox-tech.de>,
        Varad Gautam <varad.gautam@suse.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <20210514030130.3253-1-hdanton@sina.com>
 <20210515040613.12820-1-hdanton@sina.com>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <5d73d162-e860-785f-a775-1100b0aa57dd@colorfullife.com>
Date:   Sat, 15 May 2021 07:41:51 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210515040613.12820-1-hdanton@sina.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 5/15/21 6:06 AM, Hillf Danton wrote:
> On Fri, 14 May 2021 17:51:47 +0200  Manfred Spraul wrote:
>> On 5/14/21 5:01 AM, Hillf Danton wrote:
>>> The pipeline waker could start doing its job once waiter releases lock and
>>> get the work done before waiter takes a nap, so check wait condition before
>>> sleep to avoid waiting the wakeup that will never come, though that does not
>>> hurt much thanks to timer timeouts like a second.
>> First: The timeout could be infinity, thus the code must not rely on a
>> timeout wakeup.
>>
>> A wrong wait is would be a bug.
>>
>>
>>> Check signal for the same reason.
>>>
>>> Signed-off-by: Hillf Danton <hdanton@sina.com>
>>> ---
>>>
>>> --- y/ipc/mqueue.c
>>> +++ x/ipc/mqueue.c
>>> @@ -710,15 +710,24 @@ static int wq_sleep(struct mqueue_inode_
>>>    		__set_current_state(TASK_INTERRUPTIBLE);
>>>    
>>>    		spin_unlock(&info->lock);
>>> -		time = schedule_hrtimeout_range_clock(timeout, 0,
>>> -			HRTIMER_MODE_ABS, CLOCK_REALTIME);
>>>    
>> I do not see a bug:
>>
>> We do the __set_current_state() while holding the spinlock. If there is
>> a wakeup, then the wakeup will change current->state to TASK_RUNNING.
> Correct.
>> schedule() will not remove us from the run queue when current->state is
>> TASK_RUNNING. The same applies if there are pending signals: schedule()
>> checks for pending signals and sets current->state to TASK_RUNNING.
>>
>> Since the __set_current_state() is done while we hold info->lock, and
>> since the wakeup cannot happen before we have dropped the lock [because
>> the task that wakes us up needs the same lock], I do not see how a
>> wakeup could be lost.
>>
>> Thus: Which issue do you see?
> 	waiter		waker
> 	----		----
> 	unlock
> 			lock
> 	irq		set STATE_READY
> 	softirq		unlock
> 			wakeup
> 	sleep a tick
> 	  schedule();
>
> No need to schedule given READY.

This is not possible to avoid:

	waiter		waker
	----		----
	unlock
	schedule();
	  calls __schedule()
	   <before rq_lock()>

  	                lock
			set STATE_READY
			unlock
			wakeup
			--> set waiter->state = TASK_RUNNING

Now the run queue will be evaluated even though there is strictly speaking no need to do that.
Changes in ipc/sem.c can't solve that: From what I see, the majority of the critical window is in kernel/sched/*.c and not in ipc/sem.c
I do not consider it as useful to add complexity just to reduce the size of a extremely rare event.

Especially: No harm is done. User space can be preempted at any time, so the kernel is always allowed to check the run queue.

--
	Manfred