RE: [PATCH for-next 06/10] rdma: Set physical MTU for query_port function

From: "Wan, Kaike" <kaike.wan@intel.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	"dledford@redhat.com" <dledford@redhat.com>,
	"jgg@ziepe.ca" <jgg@ziepe.ca>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: RE: [PATCH for-next 06/10] rdma: Set physical MTU for query_port function
Date: Mon, 19 Apr 2021 13:09:13 +0000	[thread overview]
Message-ID: <DM6PR11MB33060BC01328357BF1D9E562F4499@DM6PR11MB3306.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YH13p0zRz+M9Tmzz@unreal>


> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Monday, April 19, 2021 8:29 AM
> To: Wan, Kaike <kaike.wan@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>;
> dledford@redhat.com; jgg@ziepe.ca; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH for-next 06/10] rdma: Set physical MTU for query_port
> function
> 
> On Mon, Apr 19, 2021 at 12:20:33PM +0000, Wan, Kaike wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> > > Sent: Thursday, April 08, 2021 8:31 AM
> > > To: Leon Romanovsky <leon@kernel.org>
> > > Cc: dledford@redhat.com; jgg@ziepe.ca; linux-rdma@vger.kernel.org;
> > > Wan, Kaike <kaike.wan@intel.com>
> > > Subject: Re: [PATCH for-next 06/10] rdma: Set physical MTU for
> > > query_port function
> > >
> > > On 4/8/2021 8:14 AM, Leon Romanovsky wrote:
> > > > On Thu, Apr 08, 2021 at 08:06:46AM -0400, Dennis Dalessandro wrote:
> > > >> On 4/1/2021 4:42 AM, Leon Romanovsky wrote:
> > > >>> On Mon, Mar 29, 2021 at 09:54:12AM -0400,
> > > dennis.dalessandro@cornelisnetworks.com wrote:
> > > >>>> From: Kaike Wan <kaike.wan@intel.com>
> > > >>>>
> > > >>>> This is a follow on patch to add a phys_mtu field to the
> > > >>>> ib_port_attr structure to indicate the maximum physical MTU the
> > > >>>> underlying device supports.
> > > >>>>
> > > >>>> Extends the following:
> > > >>>> commit 6d72344cf6c4 ("IB/ipoib: Increase ipoib Datagram mode
> > > >>>> MTU's upper limit")
> > > >>>>
> > > >>>> Reviewed-by: Mike Marciniszyn
> > > >>>> <mike.marciniszyn@cornelisnetworks.com>
> > > >>>> Signed-off-by: Kaike Wan <kaike.wan@intel.com>
> > > >>>> Signed-off-by: Dennis Dalessandro
> > > >>>> <dennis.dalessandro@cornelisnetworks.com>
> > > >>>> ---
> > > >>>>    drivers/infiniband/hw/bnxt_re/ib_verbs.c        |  1 +
> > > >>>>    drivers/infiniband/hw/cxgb4/provider.c          |  1 +
> > > >>>>    drivers/infiniband/hw/efa/efa_verbs.c           |  1 +
> > > >>>>    drivers/infiniband/hw/hns/hns_roce_main.c       |  1 +
> > > >>>>    drivers/infiniband/hw/i40iw/i40iw_verbs.c       |  1 +
> > > >>>>    drivers/infiniband/hw/mlx4/main.c               |  1 +
> > > >>>>    drivers/infiniband/hw/mlx5/mad.c                |  1 +
> > > >>>>    drivers/infiniband/hw/mlx5/main.c               |  2 ++
> > > >>>>    drivers/infiniband/hw/mthca/mthca_provider.c    |  1 +
> > > >>>>    drivers/infiniband/hw/ocrdma/ocrdma_verbs.c     |  1 +
> > > >>>>    drivers/infiniband/hw/qib/qib_verbs.c           |  1 +
> > > >>>>    drivers/infiniband/hw/usnic/usnic_ib_verbs.c    |  1 +
> > > >>>>    drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c |  1 +
> > > >>>>    drivers/infiniband/sw/siw/siw_verbs.c           |  1 +
> > > >>>>    drivers/infiniband/ulp/ipoib/ipoib_main.c       |  2 +-
> > > >>>>    include/rdma/ib_verbs.h                         | 17 -----------------
> > > >>>>    16 files changed, 16 insertions(+), 18 deletions(-)
> > > >>>
> > > >>> But why? What will it give us that almost all drivers have same
> > > >>> props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); line?
> > > >>>
> > > >>
> > > >> Almost is not all. Alternative idea to convey this? Seemed like a
> > > >> sensible thing to at least have support for but open to other
> approaches.
> > > >
> > > > What about leave it as is?
> > > >
> > > > I'm struggling to get the rationale behind this patch., the code
> > > > already works and set the phys_mtu correctly, isn't it?
> > >
> > > I see what you are saying now. Kaike, correct me if I'm wrong, but
> > > the intent of this patch is just to make everything behave the same
> > > in the sense that a device could have a different physical MTU. The
> > > field got added to the ib_port_attr previously so this is giving it an initial
> value vs leaving it unset.
> > [Wan, Kaike]  Correct.
> 
> No one is using this "phys_mtu" field, except one place in ipoib.
[Wan, Kaike]  Since phys_mtu was introduced into ib_port_attr and every query_port call will  return it in ib_port_attr. Rather than leaving it set to 0, setting it correctly in each hw driver seem more reasonable to me. 

> 
> Thanks
> 
> >
> > >
> > > -Denny
> >