Received-SPF: pass (google.com: domain of linux-kernel+bounces-186741-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <2c5fed84-bd3f-4f57-893d-59434e64b9c3@oracle.com>
Date: Wed, 22 May 2024 14:44:36 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] x86/vector: Fix vector leak during CPU offline
To: Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org
Cc: mingo@redhat.com, dave.hansen@linux.intel.com, hpa@zytor.com,
        joe.jin@oracle.com, linux-kernel@vger.kernel.org,
        virtualization@lists.linux.dev, Borislav Petkov <bp@alien8.de>
References: <875xv7idcb.ffs@tglx>
Content-Language: en-US
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <875xv7idcb.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?dURiQVBDTUJ6ZjNPL2xzamlJMkZYNWhhV1luMDhRYi8rN1FRM0JPUUYvYXV4?=
 =?utf-8?B?OTllZFlSdHRmMmxjTDRLeTFkT3I4SzhUWkhKZkU2YjJkeUZvV3BYeFBrMVoz?=
 =?utf-8?B?TG5RTTgzMnpMcDhhaEIwTmNCL0FrdVRIaldVTU90SUNDUDZSVjhla2FkWEh1?=
 =?utf-8?B?aU40UDY0SmZpUUxvNFk1S1N1RjlMc1dVbTlZTTNaMkcwY2h3emYzZ2FaVHd1?=
 =?utf-8?B?eUE1ZmZxWkt0K01ldXFhK0ZzZlA5dURya1Q5SXFIM09HN1VKWjBPSUVzdlBD?=
 =?utf-8?B?MElPZHVTVHdXOEczT05IUkZLTFBrMUZSVFBEdVk4aGxRZXFmZ1ppWW1aVlZW?=
 =?utf-8?B?Ump5c2hlNDZ1SkN0QXlIUVhGS1hhRC9seElZS0FyL3lSangwVGpYT1JyVU1t?=
 =?utf-8?B?Y3V5TkVjTmRlc2d6UnVCenpBcGg0NWRiNWtXTzR5T09hMXVXd21aeVdGSmxC?=
 =?utf-8?B?K0NWYTJ6eUVnN0w5SjNuL3FDZFg2dE5rV1VkeDY5ODdOcWlLZGh5ejh0dXZt?=
 =?utf-8?B?Zm8vdEwvVFhnVGprelBIVncxUmlNYlZ4cEszamp6Z1JaU3dHMkxzdStSVDJP?=
 =?utf-8?B?dWNsUW1IOWEzQ0phR2NHMWlQVUJTa2tDSzQ5TGV5cW1aRGQ5ZmVjUVY5Qjds?=
 =?utf-8?B?R01nOXBMd0JBQjJ0ZHA4dmI2SVpkR25ycUJqdkxsMEJXMEJITEpwRlhTNyt6?=
 =?utf-8?B?aURkbmkvVkdhU1NpckdsZnZSei9YejhnbSs2Z0lkbkpqUjRPd0dvSnJ2ck9Y?=
 =?utf-8?B?N25JdUt3ZWRmZHR4ZURCT0hPaTJvVzBoZXpGb0VpTEFTNUY1YmhwVy9ZVGJF?=
 =?utf-8?B?ZWJWb1NDR2Y5ZG44dFBDTGdmNlVJdW9YaW1HSjFHNVhUTkc2QmxUamlaQy9P?=
 =?utf-8?B?MEU4cEFLTVdISzNjT0R3TEp5c0J5ZExaN2x3eUpYbXJValFoeFByQzZJMVZE?=
 =?utf-8?B?SVZ5akRicm1PNXl6ZDZuRW1GTnZkTkZVZE14OFRLTkRqLzVUTU5BcDJFSW96?=
 =?utf-8?B?SER1WkZoa0I1M200UlJOSjNPY3VWQjlXV2o3SVY5aXhweDRMVDNtYnFwdkpS?=
 =?utf-8?B?bjc0RmJtL0NBMXUyMXJWd1JxbGN4c2dsa2cwamtXZTFmWmllUExxSGNOa3hi?=
 =?utf-8?B?RWdlWUtiVnlzdlMzSVZoN0NEM0J1QmFldHJRTHFDbXN2WTNjTytkTmtEVGZw?=
 =?utf-8?B?MXNOV2Z1aml6UFpVWHhGQzYxckYvODM3NXNVZzRBNkUwdkZubnF1VUF5ZmJm?=
 =?utf-8?B?WGlQd2M5d2JVdm5mb2NzZzBZMitFTm5LMUJKVGx4Mm55LzNRY1QrUERSUFhU?=
 =?utf-8?B?c1BidTlaMjgxRWhmdDFKV2VLVFF0elIyczRYRXhyZzBrdjdTYzVON2lSWlVF?=
 =?utf-8?B?dEJSSlVjTlNZMXNnL2xyZTVzajBCR1Zjb0ZFNktRU1BDU3ZoTWFPWDB0ZGcz?=
 =?utf-8?B?RVBFVVJNL2dGVHRJQmNra3U0SlFrSnE2SUZ3aVJzaVhKcVIyWGdIcXNpYW9E?=
 =?utf-8?B?eVRDVmJSbTRuelUwYWNYaU5Pbmk5VlRlZUVzWWlNNGIzNjlyL3BPY05aeFYw?=
 =?utf-8?B?ZzFwMlkveEI1dTVxV0Y3MlI2ZDlXMldrMkdHdXcxam5ZYk54am1rUlZwenFr?=
 =?utf-8?B?Z3FCSWVLcUFjV1F1WVlqNkF5ZWxhTTFWdUc0ZC9PbUlPc0h2dGx1WkY4aDRv?=
 =?utf-8?B?a2x6K0ZMYVRxbk1aZHk3bzNxbVIxNmYvNEZ0ZmdUR0NZakRpaGlqblBCWFFp?=
 =?utf-8?B?SVlsaDV5ZG1kRkkrU0FiNlFpcjh5aEtaQlVmWUNteVhqVXZOUUZCWm1nMnUv?=
 =?utf-8?B?dnE3bm8xVFRzRGpRTDdCSjg4NVJvUTFqTUVaaXpPbWxRbkl1dkNrSmNkWk9u?=
 =?utf-8?B?Z1FpVU95TkVMSlQ2ZEVITmUraEUwWFhRc2xja3pBamExQ0YwNkJGb3RoQzdF?=
 =?utf-8?B?L1JUck5hb1d4UTZ1ZmhIYmtxTjFYN3E3YmhPUzFXSlQ3RTJkWXh6YzdLYXFB?=
 =?utf-8?B?aXg2RWVOM1YxbktzZng5c3A0MFZuQ21RdTdSWVd5MWhSOWpuTXl1YXk4em85?=
 =?utf-8?B?ZzdIbXZpOCs0Vjc5U1NKZ0RlbFdoS0ZHNHowWFloVEMzU1k3Z3J3Q3U3YzVB?=
 =?utf-8?Q?Al2Aj0H/LuwWFVccHt4C3Dr6J?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	GNo1VcfCvqXBNi8zVBQsqBdvSSGP3Hc3zEgF995cEU/s2Hquk8nfP5citWglYecQkEBpYaAO5WmB+IdmVJpZYE+f/km9PnOnPN11OmvGt1Y83U6WwrlpGkLvORd37SY7we5tZkwZmeTg84OGMhcpPccwhmV9uw1ujkqeT7f9UJI/nMGbatyIi83AnHrDBsvbJ+XkrYZ1t7/DsvTUiuiVynukr/3MM0zOlIz9ML9H/BkKcFdFSC3BKNXiWsikhSmUmY4nzysU9dhs3/dszgtEeiEN+UXmgjEVl5wHUi3T8yUm9Ra8ncyYNzWakcVMX4OHyqKyUYi4tfudt2CieXHZ/KOKv5ow1jXpTsiz6xCi67yOsWDC3CR3ik6bpDy8gPnBmObF2NYsXtFJmYTGQTTQxCjWxlVT92R+XPsUA6lK8zLCDu/RRtsss2VRUMhJW3CYiaR2tClteLu1iHtUH4RWJODBT8nIG/E01+WW2+kW1XMkmd44yBetg3JqI6B0h1dlh7iH60TisXLHPo3ulb7enHqTRuY4gNvvkLDG2YacANaBF4Fk+dNUQp5b3AV1XClGIMwWCLNPKLHovbqBWMH6gs7d1ec+suOBh0peaguqeSo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ca2d8310-d204-4c47-c12a-08dc7aa861aa
X-MS-Exchange-CrossTenant-AuthSource: SA0PR10MB6425.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2024 21:44:39.2703
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BGIgyW0q3pCSU5Z4nNRYNIuUtkxyu948imup/uRB05ZRGDfN3FGel5q9fDm9b3cTU5kqzxaPV91B6JqQdfPqYA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5168
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-22_12,2024-05-22_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0
 suspectscore=0 mlxscore=0 adultscore=0 mlxlogscore=999 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405220151
X-Proofpoint-GUID: S5vy20evSU-cilwqJ4dNRp6-_sLEXg6v
X-Proofpoint-ORIG-GUID: S5vy20evSU-cilwqJ4dNRp6-_sLEXg6v


On 5/21/24 5:00 AM, Thomas Gleixner wrote:
> On Wed, May 15 2024 at 12:51, Dongli Zhang wrote:
>> On 5/13/24 3:46 PM, Thomas Gleixner wrote:
>>> So yes, moving the invocation of irq_force_complete_move() before the
>>> irq_needs_fixup() call makes sense, but it wants this to actually work
>>> correctly:
>>> @@ -1097,10 +1098,11 @@ void irq_force_complete_move(struct irq_
>>>  		goto unlock;
>>>  
>>>  	/*
>>> -	 * If prev_vector is empty, no action required.
>>> +	 * If prev_vector is empty or the descriptor was previously
>>> +	 * not on the outgoing CPU no action required.
>>>  	 */
>>>  	vector = apicd->prev_vector;
>>> -	if (!vector)
>>> +	if (!vector || apicd->prev_cpu != smp_processor_id())
>>>  		goto unlock;
>>>  
>>
>> The above may not work. migrate_one_irq() relies on irq_force_complete_move() to
>> always reclaim the apicd->prev_vector. Otherwise, the call of
>> irq_do_set_affinity() later may return -EBUSY.
> 
> You're right. But that still can be handled in irq_force_complete_move()
> with a single unconditional invocation in migrate_one_irq():
> 
> 	cpu = smp_processor_id();
> 	if (!vector || (apicd->cur_cpu != cpu && apicd->prev_cpu != cpu))
> 		goto unlock;

The current affine is apicd->cpu :)

Thank you very much for the suggestion!

> 
> because there are only two cases when a cleanup is required:
> 
>    1) The outgoing CPU is the current target
> 
>    2) The outgoing CPU was the previous target
> 
> No?

I agree with this statement.

My only concern is: while we use "apicd->cpu", the irq_needs_fixup() uses a
different way. It uses d->common->effective_affinity or d->common->affinity to
decide whether to move forward to migrate the interrupt.

I have spent some time reading about the discussion that happened in the year
2017 (below link). According to my understanding,
CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK always relies on CONFIG_SMP, and we do not
have the chance to encounter the issue for x86.

https://lore.kernel.org/all/alpine.DEB.2.20.1710042208400.2406@nanos/T/#u

I have tested the new patch for a while and never encountered any issue.

Therefore, I will send v2.

Thank you very much for all suggestions!

Dongli Zhang