Received-SPF: pass (google.com: domain of linux-kernel+bounces-177936-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <2b8e02cd-2f2e-4bf1-9332-6dde502c22b1@oracle.com>
Date: Mon, 13 May 2024 10:43:48 -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, Borislavbp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, joe.jin@oracle.com, linux-kernel@vger.kernel.org,
        virtualization@lists.linux.dev
References: <20240510190623.117031-1-dongli.zhang@oracle.com>
 <87msotnaow.ffs@tglx>
Content-Language: en-US
From: Dongli Zhang <dongli.zhang@oracle.com>
In-Reply-To: <87msotnaow.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?UnhrSmxHUzZzK216Y01iNE8rVkZZM0JVWmNVZ3JzdFd3dFFDcnpxU3o3bi84?=
 =?utf-8?B?dGRGOEJhZjFveUVYb09ZbGlUczk1N0xmV29wbnp4ZzJSemhFTENrd1gxRTB3?=
 =?utf-8?B?RUVTVTJYcmtPRW5BeS9CMlJJMjc4Z29xa0R2TDRDVVBVSkJoc2ROYnRVcVNY?=
 =?utf-8?B?SXFqSFd2Mm85Y2tCa0IyQTVXU1FxejVnZ05LTGJtV1dsMHhUcHl6bERDZXNL?=
 =?utf-8?B?VHBhSkJXWUp4RkdubWJyMXdWa3ovUGFkNDlyM29acnBmWFNYTjA0dzdCSFhw?=
 =?utf-8?B?Tm4wa0VBajJJenNVaUpvZ08rSDZoeFFnZGNhMFB6aUNHdVJUbXA0YWNlMGFs?=
 =?utf-8?B?L0JPbkhkQWx1M1E4NWVFTFByeCtlTitqaWpSMlFYb1ZKUDFvTWxsYytuejZV?=
 =?utf-8?B?QmpLSzBCTUhIOWJQUGMxRHQ1RE9tckE0WXVTWTJmay9IcHk3MGRMbTFPWGJJ?=
 =?utf-8?B?eERESDgyVFd6YjlxSHhMcTNMQzFHZzg0ZlkxTnZ1OUdyd01ZVUZGNTZMWmlF?=
 =?utf-8?B?dG12R2VSNkhKV1JZQXI5MmZ1Mk40Yjd2aEl4UWEzQXoxNTJVZVVObm5qZVBC?=
 =?utf-8?B?YUJHWDIva3YzOHlQV1k3QXVxbldsUjZKVVpEcHFhVDMwaEhDWTJaQTRXR2FD?=
 =?utf-8?B?VmZtWm1oeWwzNWVXYm5IRDdYcE9EbSszNWVycVB4Z2JnTDl6MmwrUjdCNVlM?=
 =?utf-8?B?ck12bG4raDNCa3BrclVPRldsWFdHd3dWSEwwSS8ya1VtWXk3elduSysza0x0?=
 =?utf-8?B?ZHUwenZwT1MvT1dVZnVacEJiOCs3UmtKbEJtaWdXcGhGSjdmT3VoQ1kwek9z?=
 =?utf-8?B?ZkdKOTRlOVJ5SXptZ0VJZENBR3lVeSt0c0J5aXVQelQyV2V3dGZqWjFpRHJs?=
 =?utf-8?B?MEJwbmhLSmtYZGVQaVIwMmFEa2o3UTR2ODZ1Y0tXSi9UMjZraHBUU1U5U213?=
 =?utf-8?B?b0dmTVoweVFabC8relFkTk1za1VxWXJuRFBkekZIZXlDYm1aWWJ5N0hueXFG?=
 =?utf-8?B?QnByWUxqeXByUkNtQlZhbyt5VXpHUEdrN0JhdGVUenRwVmh2Tm1QclFpZHo2?=
 =?utf-8?B?bFlqbFg5cThmbFhZSm5CYUlmRWxSVEtrTFhXUEo2NDdYQ0l6VW9TaUd0elBS?=
 =?utf-8?B?a0pNdTNJWTJ6TWNGeHBtUXUySVFWVWRjRFdRenA1UUxKb0xoWExvQktBeHJ5?=
 =?utf-8?B?OG5HV1g2eUlIV3owVXFBeFZPY0E0d1AwMmVNQktaUVRKWSt6dldBSjh5RSsx?=
 =?utf-8?B?R1RPaGp3dlVraENoV1ZhY0Z1VXZrMHY0TndHbU92NmN5bU5LVWRZdkhGZlY5?=
 =?utf-8?B?RThIajBBYnNqcDlhWEM4NGRpWERYUmgrZWtKVGxmaXJDUU9venVtRVVGN2lS?=
 =?utf-8?B?TjBqSyt0OTkyZ1h2Sk5LenVkMG1pTlJ4aWsvY1dVc3FNa0M0OHduMnNPVDdL?=
 =?utf-8?B?YlBZZ3dJNkV6dUNjb3k5eEx0WGp6emdUYnUvMEE4Ym1rRmcrQ0xrazNPMGNT?=
 =?utf-8?B?QnZqRFZRK0NidDRCcGVDaXJnd0FMVzNDSVcybHdaaXlWbkg1MlhMZFoyL3Ax?=
 =?utf-8?B?eFRlcUtiSFlhdDFIVUNGOWc5dlE5dUdLSHZwcWVJL0M5R2x3aTIrZUhQMkt1?=
 =?utf-8?B?OUNHYlJHZzgyVkhtL3FYejJsVmdra25RSG9JaGtmQ0FQNitCNWMxQ1FrbGV0?=
 =?utf-8?B?T1oyQ1ZSdjVYY283OHBlVFZlQWNPSHFwV2laYTI2ZzdQY3VsdmFHWFpYd1NF?=
 =?utf-8?B?SEc0blF6V0NDMjByS2NqWTV5dkM2R2lGVElDRmR2Tk9BSDExVDVKc2ZuVE8w?=
 =?utf-8?B?NXZ5MTZzR3dSQU8wWituaVRtaThnNVRmaTNhTjk1cEM4anVyblo3MndxRzV4?=
 =?utf-8?B?ZUxRdktqZkNCS1dEOXNneW9MdWhyYldkRUxjV0xvdkVCMFlialZ3cDJ5amdr?=
 =?utf-8?B?WlR2QXN4RTNMS0VNdUZ5WEhlekM1b3pKY0psR0xGc3NyaGZSNThEeElydS9j?=
 =?utf-8?B?d290dXlzdW9qQ2VwdWRxMmdJelZ6NzQwVkV6SDZuamR1VFFNTFllNjBIdWtw?=
 =?utf-8?B?eTBqeGhYSFpBZXdmeGlMbElRc1BOMWl1ckVLNW15MDJDMExHbExvSC85bnhk?=
 =?utf-8?Q?c9nqilRzRJPq4WQdhAp0DEzhK?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	iTnKavBJfNk+g9kdXeQ3Rv7YGqIlGSk3g+M/eEfRejwdgxKbMGm5glH0JjZKq8fTwe7zaOKzze4SOItzYpZ72/UxMYZPSlbD9Sm9dhmet8h4vGdgP75SXkHazbAP4PK3GYUejRoDoGy4OkFMQR3/7V1p+P31JJOqPbw+XTDPrGqmP6MhOK4gaWKkDgzHEufK8I3RlMKkUb/hXChStYcOMr90pgQl+2gMryyMFBtO1x7Sc1Y010vhqVU/mNQw+hAElihQYiGOEgUNNSFq6nuy2dMHPfHZXnAbyTZmc/CjZcjItfRJFgdenENPZ1CFHiCnhg/T9e372yOUDaiDgYFJvH7Mz01rHDDazZixdqHtV5epTEU8UTHfKUaCvlT8LjHgIm/R4MgLYy+cHpLslYJ+WsZBkNO9+jk3xBVvwarAPOJbbXArKlm2EDK549nyK7nPJGtGZddCwgik8w1OmjESrxwGEWBAMwB+ifnrwX7bUAJmxoCWTdJsPccbacLo4+0JEj7gDIn/xt3T7/qIogn5PSe7il4x5df766U64N8PbvBpEA9mFFEtrm1jCoSVoNhJQfax+kn3WtO3ydmDrDcI/DFhGVWXloOssrsxFdDazKs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5ba45784-2876-4603-9f1b-08dc73744081
X-MS-Exchange-CrossTenant-AuthSource: SA0PR10MB6425.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 May 2024 17:43:51.7264
 (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: OuA/9NNt2k076X2qy/EALTdMj11K8dh7UU6maWXqMfXkcNxvyheeQjOrLUxC9EGQMGB8OKXbkU23tR2ERx1SBA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5003
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-13_12,2024-05-10_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0
 mlxlogscore=999 phishscore=0 malwarescore=0 suspectscore=0 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2405010000 definitions=main-2405130117
X-Proofpoint-GUID: GW0wYmFQdxtYJBhz-dmjRgGvFKstORBX
X-Proofpoint-ORIG-GUID: GW0wYmFQdxtYJBhz-dmjRgGvFKstORBX

Hi Thomas,

On 5/13/24 5:44 AM, Thomas Gleixner wrote:
> On Fri, May 10 2024 at 12:06, Dongli Zhang wrote:
>> The absence of IRQD_MOVE_PCNTXT prevents immediate effectiveness of
>> interrupt affinity reconfiguration via procfs. Instead, the change is
>> deferred until the next instance of the interrupt being triggered on the
>> original CPU.
>>
>> When the interrupt next triggers on the original CPU, the new affinity is
>> enforced within __irq_move_irq(). A vector is allocated from the new CPU,
>> but if the old vector on the original CPU remains online, it is not
>> immediately reclaimed. Instead, apicd->move_in_progress is flagged, and the
>> reclaiming process is delayed until the next trigger of the interrupt on
>> the new CPU.
>>
>> Upon the subsequent triggering of the interrupt on the new CPU,
>> irq_complete_move() adds a task to the old CPU's vector_cleanup list if it
>> remains online. Subsequently, the timer on the old CPU iterates over its
>> vector_cleanup list, reclaiming vectors.
>>
>> However, if the old CPU is offline before the interrupt triggers again on
>> the new CPU, irq_complete_move() simply resets both apicd->move_in_progress
>> and apicd->prev_vector to 0. Consequently, the vector remains unreclaimed
>> in vector_matrix, resulting in a CPU vector leak.
> 
> I doubt that.
> 
> Any interrupt which is affine to an outgoing CPU is migrated and
> eventually pending moves are enforced:
> 
> cpu_down()
>   ...
>   cpu_disable_common()
>     fixup_irqs()
>       irq_migrate_all_off_this_cpu()
>         migrate_one_irq()
>           irq_force_complete_move()
>             free_moved_vector();
> 
> No?

I noticed this and finally abandoned the solution to fix at migrate_one_irq(),
because:

1. The objective of migrate_one_irq()-->irq_force_complete_move() looks to
cleanup before irq_do_set_affinity().

2. The irq_needs_fixup() may return false so that irq_force_complete_move() does
not get the chance to trigger.

3. Even irq_force_complete_move() is triggered, it exits early if
apicd->prev_vector==0.

The apicd->prev_vector can be cleared by __vector_schedule_cleanup() because
cpu_disable_common() releases the vector_lock after CPU is flagged offline.

void cpu_disable_common(void)
{
    int cpu = smp_processor_id();

    remove_siblinginfo(cpu);

    /* It's now safe to remove this processor from the online map */
    lock_vector_lock();
    remove_cpu_from_maps(cpu); ---> CPU is flagged offline
    unlock_vector_lock();      ---> release the vector_lock here!
    fixup_irqs();
    lapic_offline();
}


Therefore, the bugfix may become something like (just to demo the idea):

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 185738c72766..247a53fe9ada 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -1035,8 +1035,6 @@ static void __vector_schedule_cleanup(struct
apic_chip_data *apicd)
                        cl->timer.expires = jiffies + 1;
                        add_timer_on(&cl->timer, cpu);
                }
-       } else {
-               apicd->prev_vector = 0; // or print a warning
        }
        raw_spin_unlock(&vector_lock);
 }
diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
index 1ed2b1739363..5ecd072a34fe 100644
--- a/kernel/irq/cpuhotplug.c
+++ b/kernel/irq/cpuhotplug.c
@@ -69,6 +69,14 @@ static bool migrate_one_irq(struct irq_desc *desc)
                return false;
        }

+       /*
+        * Complete an eventually pending irq move cleanup. If this
+        * interrupt was moved in hard irq context, then the vectors need
+        * to be cleaned up. It can't wait until this interrupt actually
+        * happens and this CPU was involved.
+        */
+       irq_force_complete_move(desc);
+
        /*
         * No move required, if:
         * - Interrupt is per cpu
@@ -87,14 +95,6 @@ static bool migrate_one_irq(struct irq_desc *desc)
                return false;
        }

-       /*
-        * Complete an eventually pending irq move cleanup. If this
-        * interrupt was moved in hard irq context, then the vectors need
-        * to be cleaned up. It can't wait until this interrupt actually
-        * happens and this CPU was involved.
-        */
-       irq_force_complete_move(desc);
-
        /*
         * If there is a setaffinity pending, then try to reuse the pending
         * mask, so the last change of the affinity does not get lost. If


That's why I modify only the __vector_schedule_cleanup() as it looked simple.

I will fix in the CPU hotplug interrupt migration code.

> 
> In fact irq_complete_move() should never see apicd->move_in_progress
> with apicd->prev_cpu pointing to an offline CPU.

I think it is possible. The fact that a CPU is offline doesn't indicate
fixup_irqs() has already been triggered. The vector_lock is released after CPU
is flagged offline.

> 
> The CPU offline case in __vector_schedule_cleanup() should not even
> exist or at least just emit a warning.
> 
> If you can trigger that case, then there is something fundamentally
> wrong with the CPU hotplug interrupt migration code and that needs to be
> investigated and fixed.
> 

I can easily reproduce the issue.

1. Create a QEMU VM (12 vCPUs) with virtio-net, and 8 RX/TX queues (16 vectors).
Triggered active iperf3 multiqueue workload.

2. Affine all 16 vectors to CPU=11, and sleep 1 second.

3. Affine all 16 vectors to CPU=10.

4. Offline CPU=11, and sleep for 2-seconds.

5. Online CPU=11, goto step 2 in a loop.


After hours, the CPU=11 has many un-reclaimed vectors.

# cat /sys/kernel/debug/irq/domains/VECTOR
name:   VECTOR
 size:   0
 mapped: 47
 flags:  0x00000103
Online bitmaps:       12
Global available:   2302
Global reserved:       7
Total allocated:     122
System: 38: 0-19,21,50,128,236,240-242,244,246-255
 | CPU | avl | man | mac | act | vectors
     0   198     1     1    4  32-34,48
     1   198     1     1    4  32-35
     2   198     1     1    4  32-35
     3   199     1     1    3  32-34
     4   199     1     1    3  32-34
     5   199     1     1    3  32-34
     6   198     1     1    4  32-35
     7   200     1     1    2  32-33
     8   199     1     1    3  32,34-35
     9   198     1     1    4  32-33,36-37
    10   198     1     1    4  32-33,35,41
    11   118     1     1   84  32-49,51-110,113-115,117,119,121


I will fix in the interrupt migration code.

Thank you very much!

Dongli Zhang