Received-SPF: pass (google.com: domain of linux-kernel+bounces-101712-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6c1ac571-b758-4168-a992-3704c60dba61@amd.com>
Date: Wed, 13 Mar 2024 21:34:05 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] rcu: Reduce synchronize_rcu() delays when all wait heads
 are in use
To: Joel Fernandes <joel@joelfernandes.org>, paulmck@kernel.org,
 frederic@kernel.org, josh@joshtriplett.org, boqun.feng@gmail.com,
 rostedt@goodmis.org, mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com,
 qiang.zhang1211@gmail.com
Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
 neeraj.upadhyay@kernel.org
References: <20240313083228.233247-1-Neeraj.Upadhyay@amd.com>
 <aa221e99-bf08-4d36-aef1-07ffc5e71516@joelfernandes.org>
Content-Language: en-US
From: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
In-Reply-To: <aa221e99-bf08-4d36-aef1-07ffc5e71516@joelfernandes.org>
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?a1lqVkFINFBYUVc2eXhsWVpUSElWc1hQdUV2Wk5rQ3ViY0lLQUlHejFmVlox?=
 =?utf-8?B?VTdHT1M3QjFmM1FFQ1o0OTZuZmhWL0txS2FVdFl1R0hJbEY1YXB5NkwvbHVZ?=
 =?utf-8?B?c3N2UUFxVW56eHZHbW9xcVIxb1FXeGxiV3crU2QwMmN6UEdGSmxyaVQ5Tkta?=
 =?utf-8?B?U1pteGYzck53VUw0M3hpT3NNSDdXc1NKZFBsaTdiUEtFd0E4NU1uNnZScVdn?=
 =?utf-8?B?R1BVdFlYaG9DOGhPRTJjQStlTWhsK2paV2thRzVIM1ozckJ2TG1vNy9odmZJ?=
 =?utf-8?B?bFpPQlJWUGpZYm9kOTExdm9VTVBrWTQ3MlJFSGo2VWpiMm5WQURXVERZYk5m?=
 =?utf-8?B?eUVTMVMrTU5oVHpqcHl6SjIyRERuU3NPQ2JVMllTcUxBR1BVcEhtMURnaXBY?=
 =?utf-8?B?aVFLTXkxY0hUNWJyVHJUK1hhdVU3YWJ5UzBrZHVuUlB1Ny9aYzNmNmRwcHpF?=
 =?utf-8?B?bVhoWkphTDdPeVFoUytKeTZVY2taOFdQb0Y4ZUUxM1c0cFFITlhFdHIyaXBk?=
 =?utf-8?B?UmtMSXpDK2h1ZUF5WmFYbWNSYVNQbHFDRnBBR2l5N1JpdS9vamJtMlpUcGRv?=
 =?utf-8?B?ZWFHb1luVyswd2IrTEdsSXNwNll5Q2RGRTFJVTFXd2c1TUZCRFl2NEk1K2RQ?=
 =?utf-8?B?L3k0d1crZE9aS21mWm9PMGUxUGFHYko3ZlQzQnA5RTlYTVh0SmVDVktUTlpa?=
 =?utf-8?B?ZzBFbmljU043Zk5qVmIydDROelRaNG5mMk9Vd3lLMmIzdTRaTmtrWXlBcXBW?=
 =?utf-8?B?QzRoaFkzaWRXVk5JS0dUbjBYWmorRVhwdUtIZk9Vd09YUXJic2RvVVZKQ2Za?=
 =?utf-8?B?S2dRbVZ3TDlNOUZlZTJoV2ZESCtuOEQ4OFRTdE9NbU1HZ2RDak1NZFhaYmpL?=
 =?utf-8?B?Qkwra2owbXV1QVgwN3laMDAvUjVtanluM05HVnZBK1lZY0JkS0k3RlRCOGVL?=
 =?utf-8?B?N0JIZitmdGk5eUs1NWJkbXFyNUkwaWEzY2xsTE5XS2pkeWtDbU96eHkyRkky?=
 =?utf-8?B?RFpCMzhkOW1MR2RoemJiVnFvYjM0YXduZndxNnpJMmN3MStsdUp2eGJmNlZz?=
 =?utf-8?B?dkx1TjI4czBVSDE3dHpwRWl3YjF1YXUxZ0t5aGdlWGtsNlJQNlJua2ltU0p6?=
 =?utf-8?B?Qk9NV1IrTFBudXNaN1hjTi9TWjMxSnhlS2pMbHcvOW5JM0NzWmNGUWM4bFM3?=
 =?utf-8?B?bCtDNUFVZWZidU5hNDhJVTVTa3VWcU1WZTlnQ0dCYXFnbitrMXExV2VOUWJq?=
 =?utf-8?B?cVV3UGVjRk41Q1BhRXBJUTJVTVFMZGpSb1pUMnZmOE5lNnBxK3lGOFZsOUZW?=
 =?utf-8?B?eGFQZjdvekdLaHVKRzQ0dGpWUDd3aSt5NzZ6ZTRPN0NnMTNscmIrSmx4WitI?=
 =?utf-8?B?MDBFdVlCV1RYcWRSL3lxR3NFMlZ1MVRCMmg0R2dScEhBL3RNQlBVUHA2RjdO?=
 =?utf-8?B?dVFBK3B3M3pIZXdtdG0waWp6ZTNlY0RoWXd6ZTE3cjQzVmhhRlpHbnVmRlIv?=
 =?utf-8?B?WDdzYTMwYnRNSFh2VVZhaHVjN09lYlN1UTdWRDdJL25UcTJwMllFWlpuclpB?=
 =?utf-8?B?OW84b3VRcUZkb2NBUHBzRHFPMi84L0RFM1Vsd2l1a2tsRFlwVS9VOTY4d3B3?=
 =?utf-8?B?VVRCSWR4dFBVNWcvMzQxMTl3dlFTZ1RYK2F6czdwbXcrREZ3SWlJZUU4d3VS?=
 =?utf-8?B?NDJzUDN2TlZicVMrVHVyanJ3cTcvVXZybXRFMnc3c3FSK3VjZ3Y4TlRGaGFK?=
 =?utf-8?B?N1d0RDdqa2RGVzB5eUVMR0NualNUNFFFRk1INFhJTjF3YXk1dEsxbzFKZHoy?=
 =?utf-8?B?alVFQWRNR1RsdW5zZlNScFo4Mlc3SmlqNXFXbkN4cStnSTVQUVhFSFNiSkRJ?=
 =?utf-8?B?WWRKYzdFYmlNd0hsRWptQ2lsUC9BTUhmRDVsa2lGb21wOXNITlc0cVJMVUxG?=
 =?utf-8?B?eDIyV01IL2MrdXJKN210aFVJQnJocHloT0J6MzQ4cDR4bGdBVGFWMDI1bnNM?=
 =?utf-8?B?ZFBXNXIxVXlaTjhSeS9Temw4K1dEVFFieDI5UTB0MFJybVZFQkxWN1lBbCtH?=
 =?utf-8?B?bDdzUnNzaW9GVGRVeHI5ckN4MC9kQnV2WmVKVlF4RjdqR0xpaVpGZ0hFRm9j?=
 =?utf-8?Q?J/oITAPKUgKCMuEs2t8GUvDr8?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: afbbbf0b-6ad5-4d42-3d9d-08dc43773b06
X-MS-Exchange-CrossTenant-AuthSource: DS0PR12MB6608.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2024 16:04:15.3986
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: N+IXOpW0Jb/BjDdSCPUtb/zSUfu4uLtXyV2JI/H2PDdUxFDpZig4YE9osCNzfU9biqaUpXa6eQ37AjuQFlP36g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB7677

Hi Joel,

On 3/13/2024 8:10 PM, Joel Fernandes wrote:
> Hi Neeraj,
> 
> On 3/13/2024 4:32 AM, Neeraj Upadhyay wrote:
>> When all wait heads are in use, which can happen when
>> rcu_sr_normal_gp_cleanup_work()'s callback processing
>> is slow, any new synchronize_rcu() user's rcu_synchronize
>> node's processing is deferred to future GP periods. This
>> can result in long list of synchronize_rcu() invocations
>> waiting for full grace period processing, which can delay
>> freeing of memory. Mitigate this problem by using first
>> node in the list as wait tail when all wait heads are in use.
>> While methods to speed up callback processing would be needed
>> to recover from this situation, allowing new nodes to complete
>> their grace period can help prevent delays due to a fixed
>> number of wait head nodes.
>>
>> Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
>> ---
>>  kernel/rcu/tree.c | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 9fbb5ab57c84..bdccce1ed62f 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -1470,14 +1470,11 @@ static void rcu_poll_gp_seq_end_unlocked(unsigned long *snap)
>>   * for this new grace period. Given that there are a fixed
>>   * number of wait nodes, if all wait nodes are in use
>>   * (which can happen when kworker callback processing
>> - * is delayed) and additional grace period is requested.
>> - * This means, a system is slow in processing callbacks.
>> - *
>> - * TODO: If a slow processing is detected, a first node
>> - * in the llist should be used as a wait-tail for this
>> - * grace period, therefore users which should wait due
>> - * to a slow process are handled by _this_ grace period
>> - * and not next.
>> + * is delayed), first node in the llist is used as wait
>> + * tail for this grace period. This means, the first node
>> + * has to go through additional grace periods before it is
>> + * part of the wait callbacks. This should be ok, as
>> + * the system is slow in processing callbacks anyway.
>>   *
>>   * Below is an illustration of how the done and wait
>>   * tail pointers move from one set of rcu_synchronize nodes
>> @@ -1725,15 +1722,17 @@ static bool rcu_sr_normal_gp_init(void)
>>  		return start_new_poll;
>>  
>>  	wait_head = rcu_sr_get_wait_head();
>> -	if (!wait_head) {
>> -		// Kick another GP to retry.
>> +	if (wait_head) {
>> +		/* Inject a wait-dummy-node. */
>> +		llist_add(wait_head, &rcu_state.srs_next);
>> +	} else {
>> +		// Kick another GP for first node.
>>  		start_new_poll = true;
>> -		return start_new_poll;
>> +		if (first == rcu_state.srs_done_tail)
> 
> small nit:
> Does done_tail access here need smp_load_acquire() or READ_ONCE() to match the
> other users?
> 

As srs_done_tail is only updated in RCU GP thread context, I think it is not required.
Please correct me if I am wrong here.

> Also if you don't mind could you please rebase your patch on top of mine [1] ? I
> think it will otherwise trigger this warning in my patch:

Sure!


Thanks
Neeraj

> 
> WARN_ON_ONCE(!rcu);
> 
> Because I always assume there to be at least 2 wait heads at clean up time.
> 
> [1] https://lore.kernel.org/all/20240308224439.281349-1-joel@joelfernandes.org/
> 
> Thanks!
> 
>  - Joel
> 
> 
>> +			return start_new_poll;
>> +		wait_head = first;
>>  	}
>>  
>> -	/* Inject a wait-dummy-node. */
>> -	llist_add(wait_head, &rcu_state.srs_next);
>> -
>>  	/*
>>  	 * A waiting list of rcu_synchronize nodes should be empty on
>>  	 * this step, since a GP-kthread, rcu_gp_init() -> gp_cleanup(),