Re: [PATCH v3 for-next 07/16] IB/ipoib: Increase ipoib Datagram mode MTU's upper limit

From: Ira Weiny <ira.weiny@intel.com>
To: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Leon Romanovsky <leon@kernel.org>,
	Nathan Chancellor <natechancellor@gmail.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	dledford@redhat.com, linux-rdma@vger.kernel.org,
	Mike Marciniszyn <mike.marciniszyn@intel.com>,
	Sadanand Warrier <sadanand.warrier@intel.com>,
	Kaike Wan <kaike.wan@intel.com>
Subject: Re: [PATCH v3 for-next 07/16] IB/ipoib: Increase ipoib Datagram mode MTU's upper limit
Date: Tue, 16 Jun 2020 12:21:13 -0700	[thread overview]
Message-ID: <20200616192112.GG4160762@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <53f86386-780d-4b06-9848-f8a6eede57ee@intel.com>

On Tue, Jun 16, 2020 at 03:14:51PM -0400, Dennis Dalessandro wrote:
> On 6/16/2020 2:25 AM, Leon Romanovsky wrote:
> > On Mon, Jun 15, 2020 at 05:56:50PM -0700, Nathan Chancellor wrote:
> > > On Mon, Jun 01, 2020 at 10:57:22AM -0300, Jason Gunthorpe wrote:
> > > > On Mon, Jun 01, 2020 at 09:48:47AM -0400, Dennis Dalessandro wrote:
> > > > 
> > > > > They should probably all be in "enum ib_mtu". Jason any issues with us donig
> > > > > that? I can't for certain recall the real reason they were kept separate in
> > > > > the first place.
> > > > 
> > > > It is probably OK
> > > > 
> > > > Jason
> > > 
> > > I don't mind taking a wack at this if you guys are too busy (I'm rather
> > > tired of seeing the warning across all of my builds). However, I am
> > > wondering how far should this be unwound? Should 'enum opa_mtu' be
> > > collapsed into 'enum ib_mtu' and then all of the opa conversion
> > > functions be eliminated in favor of the ib ones? It looks like
> > > OPA_MTU_8192 and OPA_MTU_10240 are used in a few places within
> > > drivers/infiniband/hw/hfi1, should all of those instances be converted
> > > over to IB_MTU_* and the defines at the top of
> > > drivers/infiniband/hw/hfi1/hfi.h be eliminated?
> 
> My opinion is yes.
> 
> > We rather keep separation due to logic separation.
> 
> To be fair, "you" rather. Not we. I'd like some others to weigh in here.
> Increasing the available MTUs an an enum just makes sense. Why does it
> matter if IB doesn't need them right now. Maybe someday.
> 
> > While ib_* defines come from IBTA and interoperable across different
> > devices and vendors, opa_* definitions are Intel proprietary ones used
> > for the product that was canceled.
> 
> But does it hurt to have more potentially available? Can you please explain
> the technical reason here?

The problem is that the IBTA _may_ define those enums to mean something
different in the future.  Hopefully, the Intel representatives within the IBTA
would try to make them compatible but I don't know that 10K 'fits' with the
IBTA's future.  8K seems like a reasonable extension for the IBTA but again we
as Linux developers can't say that will happen for sure.  We just don't control
what the IBTA does in that regard.

Ira

> 
> -Denny
> 