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 11/13] x86/uintr: Introduce uintr_wait() syscall
To:     Thomas Gleixner <tglx@linutronix.de>, <x86@kernel.org>
CC:     Tony Luck <tony.luck@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        "Ingo Molnar" <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Andy Lutomirski <luto@kernel.org>,
        Jens Axboe <axboe@kernel.dk>,
        Christian Brauner <christian@brauner.io>,
        Peter Zijlstra <peterz@infradead.org>,
        Shuah Khan <shuah@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Jonathan Corbet <corbet@lwn.net>,
        Ashok Raj <ashok.raj@intel.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        "Gayatri Kammela" <gayatri.kammela@intel.com>,
        Zeng Guang <guang.zeng@intel.com>,
        "Dan Williams" <dan.j.williams@intel.com>,
        Randy E Witt <randy.e.witt@intel.com>,
        Ravi V Shankar <ravi.v.shankar@intel.com>,
        Ramesh Thomas <ramesh.thomas@intel.com>,
        <linux-api@vger.kernel.org>, <linux-arch@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20210913200132.3396598-1-sohil.mehta@intel.com>
 <20210913200132.3396598-12-sohil.mehta@intel.com> <87r1dedykm.ffs@tglx>
From:   Sohil Mehta <sohil.mehta@intel.com>
Message-ID: <2d404db6-828a-98de-f409-94ddf2c2af67@intel.com>
Date:   Tue, 28 Sep 2021 16:08:40 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
In-Reply-To: <87r1dedykm.ffs@tglx>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TGJQTVR5UUJicHFPdFA5TG12ZWVNT2NJd0d0bXZQWjhrZ3VoSTF0QnlWOS85?=
 =?utf-8?B?OVBYRzZLbDgrSXllNVpiVlFaZ2FHV2JBa29WYUtQL002TmQ2NmJRZkxHTG4x?=
 =?utf-8?B?bFBaUHBpTEZCeUVGVlZzeWNkNVU4LzNPR1c0aHJ1WHV1UVdsOEFPeEFKNTUy?=
 =?utf-8?B?a2dZa3pKUDJuK0tVendLbjk2K0lwM3oySU5KZU9rU3BUaVRHcHRnYU9lTFMr?=
 =?utf-8?B?Z1ZmL0RqOFVXM3E2MlRlbkMzOE4zeFVRY2Q1eG9pVTVqRjNRWHpTMDBHRVBl?=
 =?utf-8?B?MFFNTTlZYS9GU29ueWlMSGhFOGNMcnRvTzAwZzZUSkFEWEpDZ1pMWkx3dEFO?=
 =?utf-8?B?NS9EQTlzNSsvaGdaeVByOVd2VCtCKzZmOG1aODhSWnlOamdrSmkwZHMzcDVH?=
 =?utf-8?B?S3lKMzJObVA1OG9vdXMvcVpCWmluS1VhZlNqb245Snl5eUJpdU5VcDFSdzNP?=
 =?utf-8?B?cHlHV3o1RVpjYmxPbVI1U01mNnNhRDZ6Yk1XcmttK3owRzY5WUgyaHZ1NGth?=
 =?utf-8?B?aDkxM0J1L3JJUWJrQnlXaDBmOWxEeFNCSzRKODhPUUdNN0VCM1lHUXVEdjVr?=
 =?utf-8?B?TnZUakJCQjhoVDhoUzdrbVZ5d0IxY3d6MTQreUZINzlGM0ZNVDFpRUUxeHcx?=
 =?utf-8?B?R2toTGpxWW9zcU1FdGIvbENrWmI2aHRkbjJjYU5uWlU5anovbThzbDhTUkhz?=
 =?utf-8?B?amJPVWVxdmU2WU94K2VucHBIM2ZLamcxVWtwbVNUZG5FdnRSQ2xEa29KbnQr?=
 =?utf-8?B?WGs5ZkxwZG5pTGc0TXhjQXJ2WmcwekJ2SlNNc0ZGaGNCd09YczlmMEwveW9z?=
 =?utf-8?B?aDNMbnYxMSt4bFZpWGV6dDVGVFNvc3FoNTV0dW5WSXI2RDFpR3hYbFczQ2U3?=
 =?utf-8?B?RXhSRllYZ1hDZ0FYemI2bk1RUmJZSmVYdk1MaTM1cHpUM3hjT05jSXlvdDJQ?=
 =?utf-8?B?YjZzY0RVSjhBcVZFclJaMkRWM1h6S2hrVnIxK1o5eDhEUnNYVUVVcjgvL1F3?=
 =?utf-8?B?U0x2bXQyVlZTZ0diMjB0L3M3dFllUm9MeEsyRXA2aWNiY2lvU01VU1p2VmhS?=
 =?utf-8?B?UnNhSStyZUROeXRtRmFQK2wzYzBaekVIRVJjdFlSaXUwMFFuZ1JHQ0xDKzRF?=
 =?utf-8?B?bDFzUi8vN1R5Q2NleDhCZDBsYnhxbFZ0b3NnNFJFTlQ1QitqMDhNZTRjQUs5?=
 =?utf-8?B?dnpkWnF3R1VYZlJSclJjaGZEelF0L094MWhzTURqOWxuWGpKSUk4NEczSnpx?=
 =?utf-8?B?Zzhqa2diMEsrcmdhSmphZ2ZmWUwxQWluaGYwdWp0Y1FqYllMc09JQWxOSG9w?=
 =?utf-8?B?cUlIejdsOEZMdmtWdWRGMHJJU2w1eEN2ZW9MSHdHdHdEMFFERXYzdlZXZEVC?=
 =?utf-8?B?OHQ5T3V4R01UYUc3VVF1WHVRcElmd2JjTlplTjh2aGJTdmF3eUZlUHhXb2Vt?=
 =?utf-8?B?Ky9pa0pvU1VCYzN1b2RKS1NuRXQ2NFlHTUJTYXNESEFaVGIyS1BDdUJxeUFi?=
 =?utf-8?B?TmszT3kwSDk3amltRjdwcTN2WVlGUStCaVVLU08veUZ5OXdSWEt4bE9xQWM3?=
 =?utf-8?B?K1lxVERTRFZNdVpoWlQ2Sm9HZHVsT1Q5TWhrdXR3clZJSFVMV1ptVitjRm5r?=
 =?utf-8?B?K1VZUGNha3BZUnhXdEU2L3ZsbWowOUFaNXZ3TmhTNFFDWmR6R3g2SWttN0Rm?=
 =?utf-8?B?NWFhWG5KcitMQ2NjWDJBSFc5dENwUzVwSlpWSFlwSTdVUFZLdlArOWIyak92?=
 =?utf-8?Q?gOQrh3V8C2eCPnzVrxaPZRQzoHW+nwlqFGmqdR7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 86027aaf-90fd-46eb-e855-08d982d4eaa9
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2021 23:08:43.3735
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pptubdzV+TZY2zQcRYmUXui66yapp2u/Cjw1eEn7YQyc4UxGxxhvdsx/zu18iH5aBN8PY7lN/c6Q2Z99h2fzWA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3494
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 9/24/2021 4:04 AM, Thomas Gleixner wrote:
> On Mon, Sep 13 2021 at 13:01, Sohil Mehta wrote:
>> Currently, the task wait list is global one. To make the implementation
>> scalable there is a need to move to a distributed per-cpu wait list.
> How are per cpu wait lists going to solve the problem?


Currently, the global wait list can be concurrently accessed by multiple 
cpus. If we have per-cpu wait lists then the UPID scanning only needs to 
happen on the local cpu's wait list.

After an application calls uintr_wait(), the notification interrupt will 
be delivered only to the cpu where the task blocked. In this case, we 
can reduce the UPID search list and probably get rid of the global 
spinlock as well.

Though, I am not sure how much impact this would have vs. the problem of 
scanning the entire wait list.

>> +
>> +/*
>> + * Handler for UINTR_KERNEL_VECTOR.
>> + */
>> +DEFINE_IDTENTRY_SYSVEC(sysvec_uintr_kernel_notification)
>> +{
>> +	/* TODO: Add entry-exit tracepoints */
>> +	ack_APIC_irq();
>> +	inc_irq_stat(uintr_kernel_notifications);
>> +
>> +	uintr_wake_up_process();
> So this interrupt happens for any of those notifications. How are they
> differentiated?


Unfortunately, there is no help from the hardware here to identify the 
intended target.

When a task blocks we:
* switch the UINV to a kernel NV.
* leave SN as 0
* leave UPID.NDST to the current cpu
* add the task to a wait list

When the notification interrupt arrives:
* Scan the entire wait list to check if the ON bit is set for any UPID 
(very inefficient)
* Set SN to 1 for that task.
* Change the UINV to user NV.
* Remove the task from the list and make it runnable.

We could end up detecting multiple tasks that have the ON bit set. The 
notification interrupt for any task that has ON set is expected to 
arrive soon anyway. So no harm done here.

The main issue here is we would end up scanning the entire list for 
every interrupt. Not sure if there any way we could optimize this?


> Again. We have proper wait primitives.

I'll use proper wait primitives next time.
>> +	return -EINTR;
>> +}
>> +
>> +/*
>> + * Runs in interrupt context.
>> + * Scan through all UPIDs to check if any interrupt is on going.
>> + */
>> +void uintr_wake_up_process(void)
>> +{
>> +	struct uintr_upid_ctx *upid_ctx, *tmp;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&uintr_wait_lock, flags);
>> +	list_for_each_entry_safe(upid_ctx, tmp, &uintr_wait_list, node) {
>> +		if (test_bit(UPID_ON, (unsigned long*)&upid_ctx->upid->nc.status)) {
>> +			set_bit(UPID_SN, (unsigned long *)&upid_ctx->upid->nc.status);
>> +			upid_ctx->upid->nc.nv = UINTR_NOTIFICATION_VECTOR;
>> +			upid_ctx->waiting = false;
>> +			wake_up_process(upid_ctx->task);
>> +			list_del(&upid_ctx->node);
> So any of these notification interrupts does a global mass wake up? How
> does that make sense?


The wake up happens only for the tasks that have a pending interrupt. 
They are going to be woken up soon anyways.

>> +/* Called when task is unregistering/exiting */
>> +static void uintr_remove_task_wait(struct task_struct *task)
>> +{
>> +	struct uintr_upid_ctx *upid_ctx, *tmp;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&uintr_wait_lock, flags);
>> +	list_for_each_entry_safe(upid_ctx, tmp, &uintr_wait_list, node) {
>> +		if (upid_ctx->task == task) {
>> +			pr_debug("wait: Removing task %d from wait\n",
>> +				 upid_ctx->task->pid);
>> +			upid_ctx->upid->nc.nv = UINTR_NOTIFICATION_VECTOR;
>> +			upid_ctx->waiting = false;
>> +			list_del(&upid_ctx->node);
>> +		}
> What? You have to do a global list walk to find the entry which you
> added yourself?

Duh! I could have gotten the upid_ctx from the task_struct itself. Will 
fix this.

Thanks,

Sohil