Netdev Archive on lore.kernel.org
 help / color / Atom feed
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
Date: Wed, 7 Apr 2021 15:44:35 +0000
Message-ID: <BY5PR12MB4322B39A132397E661680A4BDC759@BY5PR12MB4322.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20210407151359.GI7405@nvidia.com>



> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, April 7, 2021 8:44 PM
> 
> 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:
> 
> > 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 = 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.
> 
> If it returns EOPNOTUPP then the remove is never called so if it allocated
> memory and left it allocated then it is leaking memory.
> 
I probably confused you. There is no leak today because add_one allocates memory, and later on when SA/CM etc per port cap is not present, it is unused 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.
> 
Use case is clear and explained in commit logs, i.e. to not allocate the memory 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
> 
> > 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.
> 
> If that is the reason then explain in the cover letter and provide benchmarks
I doubt it will be significant but I will do a benchmark.

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-05  5:49 [PATCH rdma-next 0/8] Generalize if ULP supported check Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 1/8] RDMA/core: Check if client supports IB device or not Leon Romanovsky
2021-04-05  6:20   ` Gal Pressman
2021-04-05  8:46     ` Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 2/8] RDMA/cma: Skip device which doesn't support CM Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 3/8] IB/cm: Skip device which doesn't support IB CM Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 4/8] IB/core: Skip device which doesn't have necessary capabilities Leon Romanovsky
2021-04-06 15:46   ` Jason Gunthorpe
2021-04-07 15:06     ` Parav Pandit
2021-04-07 15:13       ` Jason Gunthorpe
2021-04-07 15:44         ` Parav Pandit [this message]
2021-04-08 12:16           ` Jason Gunthorpe
2021-04-09 12:31             ` Parav Pandit
2021-04-05  5:49 ` [PATCH rdma-next 5/8] IB/IPoIB: Skip device which doesn't have InfiniBand port Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 6/8] IB/opa_vnic: Move to client_supported callback Leon Romanovsky
2021-04-05  5:49 ` [PATCH rdma-next 7/8] net/smc: " Leon Romanovsky
2021-04-05  5:50 ` [PATCH rdma-next 8/8] net/rds: " Leon Romanovsky
2021-04-05 16:12   ` Santosh Shilimkar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BY5PR12MB4322B39A132397E661680A4BDC759@BY5PR12MB4322.namprd12.prod.outlook.com \
    --to=parav@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=dennis.dalessandro@cornelisnetworks.com \
    --cc=dledford@redhat.com \
    --cc=jgg@nvidia.com \
    --cc=kgraul@linux.ibm.com \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mike.marciniszyn@cornelisnetworks.com \
    --cc=netdev@vger.kernel.org \
    --cc=rds-devel@oss.oracle.com \
    --cc=santosh.shilimkar@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git