| Message ID | 1617026056-50483-7-git-send-email-dennis.dalessandro@cornelisnetworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fixes for 5.13 | expand |
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? Thanks
On Thu, Apr 01, 2021 at 11:42:41AM +0300, 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? Dennis? Jason
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. -Denny
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? Thanks > > -Denny
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. -Denny
> -----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. > > -Denny
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. Thanks > > > > > -Denny >
On 4/19/2021 8:29 AM, Leon Romanovsky wrote: > 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. Today. I think it would be better to formalize the idea though and have a cleaner interface. Does this cause some problem? -Denny
> -----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 > >
On Mon, Apr 19, 2021 at 09:08:55AM -0400, Dennis Dalessandro wrote: > On 4/19/2021 8:29 AM, Leon Romanovsky wrote: > > 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. > > Today. I think it would be better to formalize the idea though and have a > cleaner interface. Does this cause some problem? Not directly, but yes. Before your change, drivers don't need to care about this field because it is not in use at all, after your change all drivers need to carry same line. This is prone to errors. Thanks > > -Denny
On Mon, Apr 19, 2021 at 01:09:13PM +0000, Wan, Kaike wrote: > > > > -----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. Yes, if the drivers set this field differently, it is not the case here. All of them (except one) set same value. If you really want to have phys_mtu be correct all the time, change query_port to set it. Something like that: query_port(): if (!phys_mtu) phys_mtu = ib_mtu_enum_to_int(..) Thanks > > > > > Thanks > > > > > > > > > > > > > -Denny > > >
On 4/20/2021 1:11 AM, Leon Romanovsky wrote: > On Mon, Apr 19, 2021 at 09:08:55AM -0400, Dennis Dalessandro wrote: >> On 4/19/2021 8:29 AM, Leon Romanovsky wrote: >>> 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. >> >> Today. I think it would be better to formalize the idea though and have a >> cleaner interface. Does this cause some problem? > > Not directly, but yes. > > Before your change, drivers don't need to care about this field because > it is not in use at all, after your change all drivers need to carry same > line. This is prone to errors. Perhaps a more common place to set this in the core is appropriate. -Denny
On Tue, Apr 20, 2021 at 08:17:00AM -0400, Dennis Dalessandro wrote: > On 4/20/2021 1:11 AM, Leon Romanovsky wrote: > > On Mon, Apr 19, 2021 at 09:08:55AM -0400, Dennis Dalessandro wrote: > > > On 4/19/2021 8:29 AM, Leon Romanovsky wrote: > > > > 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. > > > > > > Today. I think it would be better to formalize the idea though and have a > > > cleaner interface. Does this cause some problem? > > > > Not directly, but yes. > > > > Before your change, drivers don't need to care about this field because > > it is not in use at all, after your change all drivers need to carry same > > line. This is prone to errors. > > Perhaps a more common place to set this in the core is appropriate. This is basically what I suggested to Kaike. Thanks > > -Denny > >
On 4/20/2021 8:31 AM, Leon Romanovsky wrote: > On Tue, Apr 20, 2021 at 08:17:00AM -0400, Dennis Dalessandro wrote: >> On 4/20/2021 1:11 AM, Leon Romanovsky wrote: >>> On Mon, Apr 19, 2021 at 09:08:55AM -0400, Dennis Dalessandro wrote: >>>> On 4/19/2021 8:29 AM, Leon Romanovsky wrote: >>>>> 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. >>>> >>>> Today. I think it would be better to formalize the idea though and have a >>>> cleaner interface. Does this cause some problem? >>> >>> Not directly, but yes. >>> >>> Before your change, drivers don't need to care about this field because >>> it is not in use at all, after your change all drivers need to carry same >>> line. This is prone to errors. >> >> Perhaps a more common place to set this in the core is appropriate. > > This is basically what I suggested to Kaike. Yep, just seen that after I hit send.
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index ba515ef..17d4871 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -205,6 +205,7 @@ int bnxt_re_query_port(struct ib_device *ibdev, u8 port_num, port_attr->phys_state = IB_PORT_PHYS_STATE_DISABLED; } port_attr->max_mtu = IB_MTU_4096; + port_attr->phys_mtu = ib_mtu_enum_to_int(port_attr->max_mtu); port_attr->active_mtu = iboe_get_mtu(rdev->netdev->mtu); port_attr->gid_tbl_len = dev_attr->max_sgid; port_attr->port_cap_flags = IB_PORT_CM_SUP | IB_PORT_REINIT_SUP | diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c index 1f1f856..09a995b 100644 --- a/drivers/infiniband/hw/cxgb4/provider.c +++ b/drivers/infiniband/hw/cxgb4/provider.c @@ -311,6 +311,7 @@ static int c4iw_query_port(struct ib_device *ibdev, u8 port, IB_PORT_VENDOR_CLASS_SUP | IB_PORT_BOOT_MGMT_SUP; props->gid_tbl_len = 1; props->max_msg_sz = -1; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); return ret; } diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c index 479b604..1a426c0 100644 --- a/drivers/infiniband/hw/efa/efa_verbs.c +++ b/drivers/infiniband/hw/efa/efa_verbs.c @@ -261,6 +261,7 @@ int efa_query_port(struct ib_device *ibdev, u8 port, props->active_speed = IB_SPEED_EDR; props->active_width = IB_WIDTH_4X; props->max_mtu = ib_mtu_int_to_enum(dev->dev_attr.mtu); + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = ib_mtu_int_to_enum(dev->dev_attr.mtu); props->max_msg_sz = dev->dev_attr.mtu; props->max_vl_num = 1; diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c index 1a747f7..0fb46eb 100644 --- a/drivers/infiniband/hw/hns/hns_roce_main.c +++ b/drivers/infiniband/hw/hns/hns_roce_main.c @@ -228,6 +228,7 @@ static int hns_roce_query_port(struct ib_device *ib_dev, u8 port_num, /* props being zeroed by the caller, avoid zeroing it here */ props->max_mtu = hr_dev->caps.max_mtu; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->gid_tbl_len = hr_dev->caps.gid_table_len[port]; props->port_cap_flags = IB_PORT_CM_SUP | IB_PORT_REINIT_SUP | IB_PORT_VENDOR_CLASS_SUP | diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c index 375b774..0e76968 100644 --- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c +++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c @@ -104,6 +104,7 @@ static int i40iw_query_port(struct ib_device *ibdev, props->active_width = IB_WIDTH_4X; props->active_speed = 1; props->max_msg_sz = I40IW_MAX_OUTBOUND_MESSAGE_SIZE; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); return 0; } diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c index f26a0d9..a33a7b7 100644 --- a/drivers/infiniband/hw/mlx4/main.c +++ b/drivers/infiniband/hw/mlx4/main.c @@ -702,6 +702,7 @@ static int ib_link_query_port(struct ib_device *ibdev, u8 port, props->active_width = out_mad->data[31] & 0xf; props->active_speed = out_mad->data[35] >> 4; props->max_mtu = out_mad->data[41] & 0xf; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = out_mad->data[36] >> 4; props->subnet_timeout = out_mad->data[51] & 0x1f; props->max_vl_num = out_mad->data[37] >> 4; diff --git a/drivers/infiniband/hw/mlx5/mad.c b/drivers/infiniband/hw/mlx5/mad.c index 652c6cc..9a1806d 100644 --- a/drivers/infiniband/hw/mlx5/mad.c +++ b/drivers/infiniband/hw/mlx5/mad.c @@ -555,6 +555,7 @@ int mlx5_query_mad_ifc_port(struct ib_device *ibdev, u8 port, props->active_width = out_mad->data[31] & 0xf; props->active_speed = out_mad->data[35] >> 4; props->max_mtu = out_mad->data[41] & 0xf; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = out_mad->data[36] >> 4; props->subnet_timeout = out_mad->data[51] & 0x1f; props->max_vl_num = out_mad->data[37] >> 4; diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 9ff28f7..732e5ce 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -511,6 +511,7 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num, props->qkey_viol_cntr = qkey_viol_cntr; } props->max_mtu = IB_MTU_4096; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->max_msg_sz = 1 << MLX5_CAP_GEN(dev->mdev, log_max_msg); props->pkey_tbl_len = 1; props->state = IB_PORT_DOWN; @@ -1321,6 +1322,7 @@ static int mlx5_query_hca_port(struct ib_device *ibdev, u8 port, mlx5_query_port_max_mtu(mdev, &max_mtu, port); props->max_mtu = mlx5_mtu_to_ib_mtu(max_mtu); + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); mlx5_query_port_oper_mtu(mdev, &oper_mtu, port); diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c index 1a3dd07..87f7b11 100644 --- a/drivers/infiniband/hw/mthca/mthca_provider.c +++ b/drivers/infiniband/hw/mthca/mthca_provider.c @@ -164,6 +164,7 @@ static int mthca_query_port(struct ib_device *ibdev, props->active_width = out_mad->data[31] & 0xf; props->active_speed = out_mad->data[35] >> 4; props->max_mtu = out_mad->data[41] & 0xf; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = out_mad->data[36] >> 4; props->subnet_timeout = out_mad->data[51] & 0x1f; props->max_vl_num = out_mad->data[37] >> 4; diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c index 3acb5c1..d0496c1 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c @@ -167,6 +167,7 @@ int ocrdma_query_port(struct ib_device *ibdev, props->phys_state = IB_PORT_PHYS_STATE_DISABLED; } props->max_mtu = IB_MTU_4096; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = iboe_get_mtu(netdev->mtu); props->lid = 0; props->lmc = 0; diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c index 8e0de26..0534a12 100644 --- a/drivers/infiniband/hw/qib/qib_verbs.c +++ b/drivers/infiniband/hw/qib/qib_verbs.c @@ -1209,6 +1209,7 @@ static int qib_query_port(struct rvt_dev_info *rdi, u8 port_num, props->max_vl_num = qib_num_vls(ppd->vls_supported); props->max_mtu = qib_ibmtu ? qib_ibmtu : IB_MTU_4096; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); switch (ppd->ibmtu) { case 4096: mtu = IB_MTU_4096; diff --git a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c index 3705c6b..a83eaf0 100644 --- a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c +++ b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c @@ -373,6 +373,7 @@ int usnic_ib_query_port(struct ib_device *ibdev, u8 port, props->bad_pkey_cntr = 0; props->qkey_viol_cntr = 0; props->max_mtu = IB_MTU_4096; + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = iboe_get_mtu(us_ibdev->ufdev->mtu); /* Userspace will adjust for hdrs */ props->max_msg_sz = us_ibdev->ufdev->mtu; diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c index fc412cb..a40083ae 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c @@ -150,6 +150,7 @@ int pvrdma_query_port(struct ib_device *ibdev, u8 port, props->state = pvrdma_port_state_to_ib(resp->attrs.state); props->max_mtu = pvrdma_mtu_to_ib(resp->attrs.max_mtu); + props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); props->active_mtu = pvrdma_mtu_to_ib(resp->attrs.active_mtu); props->gid_tbl_len = resp->attrs.gid_tbl_len; props->port_cap_flags = diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c index e389d44..8a5df31 100644 --- a/drivers/infiniband/sw/siw/siw_verbs.c +++ b/drivers/infiniband/sw/siw/siw_verbs.c @@ -173,6 +173,7 @@ int siw_query_port(struct ib_device *base_dev, u8 port, attr->gid_tbl_len = 1; attr->max_msg_sz = -1; attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu); + attr->phys_mtu = ib_mtu_enum_to_int(attr->max_mtu); attr->active_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu); attr->phys_state = sdev->state == IB_PORT_ACTIVE ? IB_PORT_PHYS_STATE_LINK_UP : IB_PORT_PHYS_STATE_DISABLED; diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c index 2fb2fa16..4ca6150 100644 --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c @@ -1865,7 +1865,7 @@ static int ipoib_parent_init(struct net_device *ndev) priv->port); return result; } - priv->max_ib_mtu = rdma_mtu_from_attr(priv->ca, priv->port, &attr); + priv->max_ib_mtu = attr.phys_mtu; result = ib_query_pkey(priv->ca, priv->port, 0, &priv->pkey); if (result) { diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 84f7084..221c5b2 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -3338,23 +3338,6 @@ static inline int rdma_mtu_enum_to_int(struct ib_device *device, u8 port, return ib_mtu_enum_to_int((enum ib_mtu)mtu); } -/** - * rdma_mtu_from_attr - Return the mtu of the port from the port attribute. - * @device: Device - * @port_num: Port number - * @attr: port attribute - * - * Return the MTU size supported by the port as an integer value. - */ -static inline int rdma_mtu_from_attr(struct ib_device *device, u8 port, - struct ib_port_attr *attr) -{ - if (rdma_core_cap_opa_port(device, port)) - return attr->phys_mtu; - else - return ib_mtu_enum_to_int(attr->max_mtu); -} - int ib_set_vf_link_state(struct ib_device *device, int vf, u8 port, int state); int ib_get_vf_config(struct ib_device *device, int vf, u8 port,