Received-SPF: pass (google.com: domain of linux-nfs+bounces-3529-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 3 Jun 2024 09:23:10 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Guoqing Jiang <guoqing.jiang@linux.dev>
Cc: cel@kernel.org, linux-nfs@vger.kernel.org, linux-rdma@vger.kernel.org
Subject: Re: [PATCH 1/2] svcrdma: Refactor the creation of listener CMA ID
Message-ID: <Zl3DvlqRGasKmhz8@tissot.1015granger.net>
References: <20240531131550.64044-4-cel@kernel.org>
 <20240531131550.64044-5-cel@kernel.org>
 <9ae0657b-b430-9318-4e19-eae9f40307fb@linux.dev>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9ae0657b-b430-9318-4e19-eae9f40307fb@linux.dev>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?pv26CWztakKSU7j2PSI3Mv7xiRKQAxYiL6CgK9MXqTtN+ba5/FOXsOB81b7T?=
 =?us-ascii?Q?+M8Nj3IBY1Q3FKsT1HSWrdvmqim5kk8hf3wRVYVcl/kJoajiTUO3OpYM8rSy?=
 =?us-ascii?Q?wNGrcGzCinyqe5arY+oWErIPCbn0szHR+ri4LqyGoR7GSGUv/e8WXQNS9FMJ?=
 =?us-ascii?Q?liutng8aQkjJ8pZrGGppbPaEk+HyuUBETiVHcImvSV0WNiaaiYyxu49zBkYV?=
 =?us-ascii?Q?1A+kTg8u5JX7Vajyso5aBcTZNX6UE324lDkqpH8O9JzHSCmuPnMFhqNnY3qj?=
 =?us-ascii?Q?G16SBtJ6Qrj9d+N6HPgN86BwUCJpmOGTX5+NikpRbB5Jc3s8ZWaq06cIHslG?=
 =?us-ascii?Q?MtG+cmDn8ILRHTb59I50HgINU8fEok69He+0ANd5zXLeHHOnReG5BbjyyfR3?=
 =?us-ascii?Q?MQ30GEjI5tOQW7JOavzZd/Pf2cBOi4SXY2d0f4zN5KVclKS+kmqPAjlAhnKK?=
 =?us-ascii?Q?r14Qpf/5o/dc+zaMzrFBKmtBrDMTO/dSXC+T0KaNvSQXH8hvJNqKOe69P8zw?=
 =?us-ascii?Q?MYGIPcS4vfvAlS1Z7DcQ4ExgTcRiLoYvrCmIGBXuDJTZx2PP9HvbNTBk8ISz?=
 =?us-ascii?Q?Iva8cjpoFTeMqlIV6Aog6bCfyY/AFitPgKuCkE5DqM3OIGphCoI1s8LJcS/C?=
 =?us-ascii?Q?3JyGjXMIOAWT6SOFLhh176i8NN0wXaMEmB8Pti+DWqCkSMpR2T+7vhM6Onmo?=
 =?us-ascii?Q?H8quzGVt63nz7jZIH+6IvB8k/ENJSTfY7NAoI5BO/wjUE7h8j3D9tA8ULttV?=
 =?us-ascii?Q?c40qwt0DRl+o5CpKE6CWMiNCV+cR9ZhjPI2gsAPZ3PPOcmIkKCWPHnChFq6Y?=
 =?us-ascii?Q?JZnqoJsz024U80OTc1/D706RKJxHHu2y5qWi0C5FumTJK4txkn8amknBBQo4?=
 =?us-ascii?Q?fre7HDBrm7Frtz5tuWgg4+xHXnfCE868vtYUmEmnxnrGxmThd+Ufmnoc24uP?=
 =?us-ascii?Q?a/H9Sr1K3UTiCjeK+M+dhvCrhfc943+VdkOXoU/4rpfRv4bkdQd1yOrXWUaj?=
 =?us-ascii?Q?t4QiXq0owY0mduq7bLjt734rqIwSjzr/ATklLmpx7Vzt0bm9K3VhuC6UW9Pj?=
 =?us-ascii?Q?rXuG4YhdgxbUw7UlYbpLqvBDNHBjNZrEcJxQB/ef+bUdB+s/z3Dz6HJYS/VH?=
 =?us-ascii?Q?ko2EmrLiJQL0ZS/NBvT/Y+XiPKietJPhfdfYRX+ZnrwFQ/3mXj/qvKJXg2Un?=
 =?us-ascii?Q?U/6wMwPGk743CRbD/juLZ6E0pjb49Ar0rZOhjJAaEhWD5uGXCnVZPbPImIjz?=
 =?us-ascii?Q?PHgdpTYppZyNY0TYXdQN1Xx05VNByyPu5whLS/hyEdemh7GPOXo+IWzPLSv8?=
 =?us-ascii?Q?XWJ4frGiI0e2dscPDx4/FlEdnEL3VXYU+k1tWqa7+X8nyjWXEiHcGKomthvL?=
 =?us-ascii?Q?958pDWhmQTdVMW6C8/5Wu5QORqVAup+bxoacozXaVHRdGoWj9ftT+T2MwlPF?=
 =?us-ascii?Q?6TnJpGiBUR4JMhyn4sXVzaRKcvOYPLhA5C95Mj4vXKrs5YP17s9lyXQGee2R?=
 =?us-ascii?Q?mrbPskieC3/0eQFiygv8vs1sdrm2GF1yF7igclpjrp+9uysXzz0VLnDtbqkl?=
 =?us-ascii?Q?KVKMLORmywX47u3hVYFjYjA9byJqM4VTAk+pzHUePwO/Upve0cTTTNt4wrhv?=
 =?us-ascii?Q?lg=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	zFk6+eUBw6/aduVVxnKKZhbweKJUGbkTss3mjEUpwVaGIua0L4tyrSASmu8BZlrDSBMEmf8ovhcA2d+fpLCH1SadZ7WxJ/Pjt1zsqXdMRphzpvfljei8ZV13qTsQQQIXKcH8bCQ8VWMo+kmXjqtTxmj76nD2TubMT2cToyIlBv0/LWsLBcAGKKzAaV4vfXhHZCIJg3SXHFkGRB72bMxCLd72dSLFKOtRc/NzXtoMA+Tmm72JFXfxxfbbulVXwubfclkmyU9M9sMVDuCHeWOgdQgXXNUG6uFLx9Wz74/iaIZKxaWvGAluaVl3SRjE+uKsY1aIUrQ4JxOovRVre3TLK0+7HszKiFABqGbDJ4m3Y+u04T7ctUyeYTwf68ZmZqs8cpr5n+1QASGzexV/orhjZ6ROQT6O23nSVP4LHi3m7R8vFlq3FfCcodeQzZht1RzOzyc3WKX+VZQuKD+ku2zQvLupwMF5Z4fUspDw4yCRnoFrz087MB3R3Ik6G0dHCv/aw97m67hpIqquFIbuqu5/mt8dHYIjQcUqVXcQEVBG080CfEx6JpdL73S7k9mnoa1eN412rCJLWll/2tex4OKnWMPmKSLXwbt9JW1oZ5C5wN0=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a144922-705b-4ac9-c909-08dc83d05210
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jun 2024 13:23:13.4139
 (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: jIvX5DxeKyOu8Uv1CTX7GWEqAeDAU6fC1I4Q+O/FSH2JunXrtqBttFyeywmVkvXM4unDp6I/aBAnTsPRvnbykw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR10MB7909
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-06-03_09,2024-05-30_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 adultscore=0 spamscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2406030112
X-Proofpoint-GUID: UFcTlRP5Kbwo1GsXaDEK7cGEJezgs7_M
X-Proofpoint-ORIG-GUID: UFcTlRP5Kbwo1GsXaDEK7cGEJezgs7_M

On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
> 
> 
> On 5/31/24 21:15, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > In a moment, I will add a second consumer of CMA ID creation in
> > svcrdma. Refactor so this code can be reused.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >   net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
> >   1 file changed, 40 insertions(+), 27 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > index 2b1c16b9547d..fa50b7494a0a 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > @@ -65,6 +65,8 @@
> >   static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
> >   						 struct net *net, int node);
> > +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> > +				   struct rdma_cm_event *event);
> >   static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> >   					struct net *net,
> >   					struct sockaddr *sa, int salen,
> > @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
> >   	}
> >   }
> > +static struct rdma_cm_id *
> > +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
> > +			  void *context)
> > +{
> > +	struct rdma_cm_id *listen_id;
> > +	int ret;
> > +
> > +	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
> > +				   RDMA_PS_TCP, IB_QPT_RC);
> > +	if (IS_ERR(listen_id))
> > +		return listen_id;
> 
> I am wondering if above need to return PTR_ERR(listen_id),

PTR_ERR would convert the listen_id error to an integer, but
svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
using PTR_ERR() would be wrong in this case.


> and I find some
> callers (in net/rds/, nvme etc)
> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
> with ret is set to PTR_ERR(id).

These functions use PTR_ERR only when the calling function returns
an int.

-- 
Chuck Lever