Received-SPF: pass (google.com: domain of linux-kernel+bounces-104455-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 15 Mar 2024 14:32:12 +0100
From: Michal Kubiak <michal.kubiak@intel.com>
To: Yewon Choi <woni9911@gmail.com>
CC: Allison Henderson <allison.henderson@oracle.com>, "David S. Miller"
	<davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
	<kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, <netdev@vger.kernel.org>,
	<linux-rdma@vger.kernel.org>, <rds-devel@oss.oracle.com>,
	<linux-kernel@vger.kernel.org>, "Dae R. Jeong" <threeearcat@gmail.com>
Subject: Re: [PATCH net v2] rds: introduce acquire/release ordering in
 acquire/release_in_xmit()
Message-ID: <ZfRN3A2yiWwAtIFf@localhost.localdomain>
References: <ZfQUxnNTO9AJmzwc@libra05>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZfQUxnNTO9AJmzwc@libra05>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tA+TAA5atzhTulFGsN+X4GE0WUkQcGt0UdJGQlreFoqYLDrBKMXIJ3xN9yOK?=
 =?us-ascii?Q?NAHA+qCo/O5xVM1z44Ap4pew81EL2EW03Ap7hQ1o9UNrzXWsYpq2lp4BZ4QB?=
 =?us-ascii?Q?43UgT/GVY7HGp+4d+nkKzN2UZbEhVzloIhJ+EyB9Ywls3uR+3tIG7EYhzSj0?=
 =?us-ascii?Q?Sk96g84IK1Gk4III2rNW4ooSGp9kuZyAHlCqe0mod5XPuWCr1FZ97Piuw8Rf?=
 =?us-ascii?Q?mKV2OJ6y8HswDbatt6+GKEhRLWnfFKr8yGwAYdHGAEYDVHYZUWLfmqxQCfMF?=
 =?us-ascii?Q?iAOlZE20c7Pchlb5MMDP0EhouKj0htJKbNl2mSRlhN7Y3d+O7wsLZWO5NltO?=
 =?us-ascii?Q?5iwDaw1OlbCuB3/KWHuWAiS5qKlUo2ZPuIMs2ArTlHh2mqeQH8KuT+Uq3Wgx?=
 =?us-ascii?Q?wxm6I1DwNSP7mzkzCQZnBa0R7mvJAuaHDqHMVGXt9Lu+K8KQVCuqziA7mXiq?=
 =?us-ascii?Q?W9wWVjJPEg7p2whgCHAnFBmnFKo+WZ3L8nZCG9VGqOLpkOP/BxIAHzVEpzEv?=
 =?us-ascii?Q?WCCIpbuvsx9DjrMLj7pCU0yvdYJBROPhPXzpwcRTfroO5zr/HKtaZuCzXgRX?=
 =?us-ascii?Q?9gIt/eMmrOLCqfkt8J0iOY7KrvUrlr4KMCqlKWybgE/329iMYmqL36zkaiMi?=
 =?us-ascii?Q?5vuI7RqATTRUQ5PKASr94rl281fcN2t7WM6UTGiBwwRqQi6F50U2eaZu+M3Z?=
 =?us-ascii?Q?HqIYj88T5SrjsbktzwAMt64u7EA6g8cNoOufIlUcSZwPxYIVm5JO4iVjjdjG?=
 =?us-ascii?Q?f8LGH7wLouUyKT0LLwKMh/VYNjLvSik25pjTevpL9cyZeAxC5+PCHmMktobK?=
 =?us-ascii?Q?07TZE+1309Sme48UBNu3Iibhgo4vPtuT60G8LUga1/KP3ll1359eEsBcamva?=
 =?us-ascii?Q?Wnx9mGGGe8dJMqoFo2M6CG+Qfb75SZAYbw7ZYU47xH6kV4cIVNbZRyeDE4cY?=
 =?us-ascii?Q?LSM7lm3Z3byBgeIawa3Rjr+dd3YaZyFSVV9xF3lqYwLqjx/SxScVoRauLgLP?=
 =?us-ascii?Q?O1YxTgn5x5QghtIrl7HMlenGdlVcD8MH5yaQCCBq+f7/oUio/y7GVkxA0O+G?=
 =?us-ascii?Q?po6ywyymaUgyUGnXGBXBahj7ICEwTZdzwPrCozCESvu4iGIDG5fMvWDoMuae?=
 =?us-ascii?Q?IKYxxHSftJXXw3asJcHKCVL/14mV5DGdJQcVcu7IwIFPKulvVWQVCGKUVcGx?=
 =?us-ascii?Q?N38NzdbC1hGb/urLLCS0d5cRpC+hUdcI0Z+TctQykbhuuDV2dV2w0idP6UOV?=
 =?us-ascii?Q?mHpJfH5p4ayVakhvj38A//wc70oNP6B3girnI8y/VnlgJnAxFThldPGWTwch?=
 =?us-ascii?Q?Cz6SIPVQ1T+GKGcnE/j1SqTdvIPY3ORBAyf7EQx1UavS1BHFbbWRuxxexAW7?=
 =?us-ascii?Q?kgUEN3Z+Q1mEZqsE39x+ZqPaSP4skry5kTXCB9rAksiW/LWOi7klynvi/FYx?=
 =?us-ascii?Q?MFPmBPwcvM0h+2sVMNXTsXKvSXFrmScSPCXbfQkWSIpUGU1rbABs2x/MZBzb?=
 =?us-ascii?Q?e+h9WMQ0d/8/t/j//AML+1lCEIIh/EGpg9BkcKnrmNeZR+8EjldAwYnvhiBE?=
 =?us-ascii?Q?IQ0bWJKIcHkajaCl7w7OwqsACQY2wyAyIuOnd5qd9SM9Obd7c4WN0309Ow9i?=
 =?us-ascii?Q?qQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ed8f403c-4094-4e9e-9391-08dc44f456af
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5782.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2024 13:32:19.7987
 (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: to8sbQuVCnRiFSItghO1TvTQXwir/iMeDumZAZkvL00ktvrdsMLEKD4ocmOL4iFa40EmXmo/JtJHzL+yvkagsQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7868
X-OriginatorOrg: intel.com

On Fri, Mar 15, 2024 at 06:28:38PM +0900, Yewon Choi wrote:
> acquire/release_in_xmit() work as bit lock in rds_send_xmit(), so they
> are expected to ensure acquire/release memory ordering semantics.
> However, test_and_set_bit/clear_bit() don't imply such semantics, on
> top of this, following smp_mb__after_atomic() does not guarantee release
> ordering (memory barrier actually should be placed before clear_bit()).
> 
> Instead, we use clear_bit_unlock/test_and_set_bit_lock() here.
> 
> Fixes: 0f4b1c7e89e6 ("rds: fix rds_send_xmit() serialization")
> Fixes: 1f9ecd7eacfd ("RDS: Pass rds_conn_path to rds_send_xmit()")
> Signed-off-by: Yewon Choi <woni9911@gmail.com>
> ---
> Changes in v1 -> v2:
> - Added missing Fixes tags
> 
>  net/rds/send.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/rds/send.c b/net/rds/send.c
> index 5e57a1581dc6..8f38009721b7 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -103,13 +103,12 @@ EXPORT_SYMBOL_GPL(rds_send_path_reset);
>  
>  static int acquire_in_xmit(struct rds_conn_path *cp)
>  {
> -	return test_and_set_bit(RDS_IN_XMIT, &cp->cp_flags) == 0;
> +	return test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags) == 0;
>  }
>  
>  static void release_in_xmit(struct rds_conn_path *cp)
>  {
> -	clear_bit(RDS_IN_XMIT, &cp->cp_flags);
> -	smp_mb__after_atomic();
> +	clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
>  	/*
>  	 * We don't use wait_on_bit()/wake_up_bit() because our waking is in a
>  	 * hot path and finding waiters is very rare.  We don't want to walk
> -- 
> 2.43.0
> 

LGTM

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>