Re: fuse uring / wake_up on the same core

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Hillf,

On 3/25/2023 5:58 AM, Hillf Danton wrote:
> On 24 Mar 2023 22:44:16 +0000 Bernd Schubert <bschubert@ddn.com>
>> How much of hack is this patch?
> 
> It adds a churn to my mind then another RFC [1] rises.
> 
> Feel free to make it work for you and resend it.
> 
> [1] Subject: [RFC PATCH 0/5] sched: Userspace Hinting for Task Placement
>     https://lore.kernel.org/lkml/20220910105326.1797-1-kprateek.nayak@amd.com/

Thank you for pointing to my series.

Another possible way to tackle this is with a strategy Andrei is using in
his "seccomp: add the synchronous mode for seccomp_unotify" series
(https://lore.kernel.org/lkml/20230308073201.3102738-1-avagin@google.com/)

In patch 2, Andrei adds a WF_CURRENT_CPU that allows the task to always
wake on the CPU where the waker is running.
(https://lore.kernel.org/lkml/20230308073201.3102738-3-avagin@google.com/)

I believe Bernd's requirement calls for a a WF_PREV_CPU which always
wakes up the task on the CPU where it previously ran. I believe you can
modify fuse_request_end() (in fs/fuse/dev.c) to use the WF_PREV_CPU flag
when waking the tasks on req->waitq

(P.S. I'm not familiar with the fuse subsystem but the comment
"Wake up waiter sleeping in request_wait_answer()" in fuse_request_end()
seems to suggest that is the right place to add this flag.)

Peter favored wake flag strategy of fixing wakeup target in a reply to an
earlier version of Andrei's series
(https://lore.kernel.org/lkml/Y8UgBnsaGDUJKH5A@hirez.programming.kicks-ass.net/)
but I'll let Peter respond with what he thinks is the right way to tackle
this.

> 
>>
>> [RFC] fuse: wake on the same core / disable migrate before wait
>>
>> From: Bernd Schubert <bschubert@ddn.com>
>>
>> Avoid bouncing cores on wake, especially with uring everything
>> is core affine - bouncing badly decreases performance.
>> With read/write(/dev/fuse) it is not good either - needs to be tested
>> for negative impacts.
>> ---
>>   fs/fuse/dev.c |   16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index e82db13da8f6..d47b6a492434 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -372,12 +372,17 @@ static void request_wait_answer(struct fuse_req *req)
>>   	struct fuse_iqueue *fiq = &fc->iq;
>>   	int err;
>>
>> +	/* avoid bouncing between cores on wake */
>> +	pr_devel("task=%p before wait on core: %u wake_cpu: %u\n",
>> +		 current, task_cpu(current), current->wake_cpu);
>> +	migrate_disable();
>> +
>>   	if (!fc->no_interrupt) {
>>   		/* Any signal may interrupt this */
>>   		err = wait_event_interruptible(req->waitq,
>>   					test_bit(FR_FINISHED, &req->flags));
>>   		if (!err)
>> -			return;
>> +			goto out;
>>
>>   		set_bit(FR_INTERRUPTED, &req->flags);
>>   		/* matches barrier in fuse_dev_do_read() */
>> @@ -391,7 +396,7 @@ static void request_wait_answer(struct fuse_req *req)
>>   		err = wait_event_killable(req->waitq,
>>   					test_bit(FR_FINISHED, &req->flags));
>>   		if (!err)
>> -			return;
>> +			goto out;
>>
>>   		spin_lock(&fiq->lock);
>>   		/* Request is not yet in userspace, bail out */
>> @@ -400,7 +405,7 @@ static void request_wait_answer(struct fuse_req *req)
>>   			spin_unlock(&fiq->lock);
>>   			__fuse_put_request(req);
>>   			req->out.h.error = -EINTR;
>> -			return;
>> +			goto out;
>>   		}
>>   		spin_unlock(&fiq->lock);
>>   	}
>> @@ -410,6 +415,11 @@ static void request_wait_answer(struct fuse_req *req)
>>   	 * Wait it out.
>>   	 */
>>   	wait_event(req->waitq, test_bit(FR_FINISHED, &req->flags));
>> +
>> +out:
>> +	migrate_enable();
>> +	pr_devel("task=%p after wait on core: %u\n", current, task_cpu(current));
>> +
>>   }
>>
>>   static void __fuse_request_send(struct fuse_req *req)
>>
--
Thanks and Regards,
Prateek