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

From: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: "Wan, Kaike" <kaike.wan@intel.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: Tue, 20 Apr 2021 08:17:00 -0400	[thread overview]
Message-ID: <0bd262f3-0eaa-b508-2a01-5ad89147538e@cornelisnetworks.com> (raw)
In-Reply-To: <YH5ilM02U4cuYK3x@unreal>

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