Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Jason Gunthorpe <jgg@nvidia.com>
To: Parav Pandit <parav@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 12:13:59 -0300
Message-ID: <20210407151359.GI7405@nvidia.com> (raw)
In-Reply-To: <BY5PR12MB4322E477DA2334233EAD5173DC759@BY5PR12MB4322.namprd12.prod.outlook.com>

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.

If you are saying 32k bytes of temporary allocation matters during
device startup then it needs benchmarks and a use case.

> > 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

Jason

  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 [this message]
2021-04-07 15:44         ` Parav Pandit
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=20210407151359.GI7405@nvidia.com \
    --to=jgg@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=dennis.dalessandro@cornelisnetworks.com \
    --cc=dledford@redhat.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=parav@nvidia.com \
    --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