Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Parav Pandit <parav@nvidia.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
CC:     Leon Romanovsky <leon@kernel.org>,
        Doug Ledford <dledford@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Karsten Graul <kgraul@linux.ibm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "rds-devel@oss.oracle.com" <rds-devel@oss.oracle.com>,
        Santosh Shilimkar <santosh.shilimkar@oracle.com>
Subject: RE: [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have
 necessary capabilities
Thread-Topic: [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have
 necessary capabilities
Thread-Index: AQHXKd+XJ6WIY5+vZk6+h+s0LuIetKqnpFkAgAGBvmCAAAdugIAABvxw
Date:   Wed, 7 Apr 2021 15:44:35 +0000
Message-ID: <BY5PR12MB4322B39A132397E661680A4BDC759@BY5PR12MB4322.namprd12.prod.outlook.com>
References: <20210405055000.215792-1-leon@kernel.org>
 <20210405055000.215792-5-leon@kernel.org> <20210406154646.GW7405@nvidia.com>
 <BY5PR12MB4322E477DA2334233EAD5173DC759@BY5PR12MB4322.namprd12.prod.outlook.com>
 <20210407151359.GI7405@nvidia.com>
In-Reply-To: <20210407151359.GI7405@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c25be7f-1f1c-4697-2de1-08d8f9dc0be3
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Apr 2021 15:44:35.9757
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ptQasQV48VV2ngKaeamGiKTOPEmfbf7/YNvLoKG/2v1wH9ukqjuyLKGC9Xco7mXzbWed0lMrF6lRTmcKcOI9cA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4949
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, April 7, 2021 8:44 PM
>=20
> On Wed, Apr 07, 2021 at 03:06:35PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, April 6, 2021 9:17 PM
> > >
> > > On Mon, Apr 05, 2021 at 08:49:56AM +0300, Leon Romanovsky wrote:
> > > > @@ -2293,6 +2295,17 @@ static void ib_sa_event(struct
> > > > ib_event_handler
> > > *handler,
> > > >  	}
> > > >  }
> > > >
> > > > +static bool ib_sa_client_supported(struct ib_device *device) {
> > > > +	unsigned int i;
> > > > +
> > > > +	rdma_for_each_port(device, i) {
> > > > +		if (rdma_cap_ib_sa(device, i))
> > > > +			return true;
> > > > +	}
> > > > +	return false;
> > > > +}
> > >
> > > This is already done though:
>=20
> > It is but, ib_sa_device() allocates ib_sa_device worth of struct for
> > each port without checking the rdma_cap_ib_sa().  This results into
> > allocating 40 * 512 =3D 20480 rounded of to power of 2 to 32K bytes of
> > memory for the rdma device with 512 ports.  Other modules are also
> > similarly wasting such memory.
>=20
> If it returns EOPNOTUPP then the remove is never called so if it allocate=
d
> memory and left it allocated then it is leaking memory.
>=20
I probably confused you. There is no leak today because add_one allocates m=
emory, and later on when SA/CM etc per port cap is not present, it is unuse=
d left there which is freed on remove_one().
Returning EOPNOTUPP is fine at start of add_one() before allocation.

> If you are saying 32k bytes of temporary allocation matters during device
> startup then it needs benchmarks and a use case.
>=20
Use case is clear and explained in commit logs, i.e. to not allocate the me=
mory which is never used.

> > > The add_one function should return -EOPNOTSUPP if it doesn't want to
> > > run on this device and any supported checks should just be at the
> > > front - this is how things work right now
>=20
> > I am ok to fold this check at the beginning of add callback.  When
> > 512 to 1K RoCE devices are used, they do not have SA, CM, CMA etc caps
> > on and all the client needs to go through refcnt + xa + sem and unroll
> > them.  Is_supported() routine helps to cut down all of it. I didn't
> > calculate the usec saved with it.
>=20
> If that is the reason then explain in the cover letter and provide benchm=
arks
I doubt it will be significant but I will do a benchmark.