Message-ID: <380b05bf-a18e-1f20-7e8e-10b61f77dec7@redhat.com>
Date:   Thu, 15 Dec 2016 11:28:06 -0500
From:   Doug Ledford <dledford@...hat.com>
To:     "ira.weiny" <ira.weiny@...el.com>,
        Leon Romanovsky <leon@...nel.org>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     "Vishwanathapura, Niranjana" <niranjana.vishwanathapura@...el.com>,
        linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
        dennis.dalessandro@...el.com
Subject: Re: [RFC v2 00/10] HFI Virtual Network Interface Controller (VNIC)

On 12/15/2016 9:52 AM, ira.weiny wrote:
> On Thu, Dec 15, 2016 at 11:12:26AM +0200, Leon Romanovsky wrote:
>> On Wed, Dec 14, 2016 at 11:59:32PM -0800, Vishwanathapura, Niranjana wrote:
>>> Thanks Jason for the valuable feedback.
>>> Here is the revised HFI VNIC patch series.
>>>
>>> ChangeLog:
>>> =========
>>> v1 => v2:
>>> a) Removed hfi_vnic bus, instead make hfi_vnic driver an 'ib client',
>>>    as per feedback from Jason Gunthorpe.
>>> b) Interface changes, data structure changes and variable name changes
>>>    associated with (a).
>>> c) Add hfi_ibdev abstraction to provide VNIC control operations to
>>>    hfi_vnic client.
>>> d) Minor fixes
>>> e) Moved hfi_vnic driver from .../sw/intel/vnic/hfi_vnic to
>>>    .../sw/intel/hfi_vnic.
>>
>> To put it into proportion, Jason asked you to do different thing.
>> http://marc.info/?l=linux-rdma&m=147977108302151&w=2
>> http://marc.info/?l=linux-rdma&m=148000415401842&w=2
>>
>> And Christoph,
>> http://marc.info/?l=linux-rdma&m=147985587425861&w=2
> 
> Understood.  However, we never heard back from Niranjanas analysis of the code
> which stated that > 60% of the code was dealing with the OPA MADs used to
> configure this device.
> 
> https://www.spinics.net/lists/linux-rdma/msg43579.html
> 
> Furthermore, neither Dave nor Doug has had time to weigh in on what we should
> do.
> 
> So before we make that change we wanted to get consensus on using the
> hfi1_ibdev abstraction rather than the bus.  This was the _real_ technical
> change.
> 
> Beyond that it is really just which maintainer wants this driver.  To that end
> I've also cc'ed Jeff Kirsher who maintains drivers/net/ethernet/intel.  Perhaps
> Dave would like the driver to go through that tree?
> 
> 
> I think there are pros and cons to both subtrees and in the end we will do
> whatever is decided.
> 
> For maintainer review:
> 
> 	1) The driver encapsulates ethernet packets with OPA headers
> 
> 	2) VNIC uses OPA management packets (MADs) for its configuration
> 
> 	3) A significant portion (> 60% +) of the code is specific to OPA
> 
> 		https://www.spinics.net/lists/linux-rdma/msg43579.html
> 
> 	4) The driver is from Intel and we expect Intel to be the primary
> 	   contributor to the code.
> 
> 	5) The driver, like hfi1, is dual licensed (GPL/BSD)
> 
> 	6) Based on Christophs feedback we will be adding device capability
> 	   bits to the IB core to indicate HFI VNIC support.
> 
> 		https://www.spinics.net/lists/linux-rdma/msg44113.html
> 
> 
> Doug, Dave, Jeff any thoughts?
> 
> Ira
> 

Sorry for my late reply.  The series is relatively large, and also
tagged with RFC, so it got shuffled to the back burner while I worked on
the stuff for this pull request.

I just read through the comments in the V1 series between Jason et. al.,
and my take on things is like this:

1) Since your intent is to make this work with multiple versions of the
hfi drivers, I disagree with Jason that just because there is only one
driver today that we should keep it simple.  Design it right from the
beginning of multi driver is your intent is, IMO, a better way to go.
You'll work out the bugs in the initial implementation and when it comes
time to add the second driver, things will go much more smoothly.

2) With more than 60% of the code being MAD related, and another
significant chunk being hfi related, and only a minor bit (20% maybe?)
being net related, I disagree that this belongs in the drivers/net or
net/ directories.  Part of the purpose of putting code like this in any
given directory is to group it with what it is most tightly tied too.
That way people doing sub-tree wide changes know the rough scope of
their work as the code that needs changed is grouped together.  Putting
this or IPoIB in one of the net trees would make it obvious to the
casual coder that these need changed for net changes, but would totally
hide the fact that once you tear into these drivers, there is a lot more
IB to them than there is net.  What's more, when 60+% of driver is
non-net, then you end up having many more of my patches crossing over
into Dave's tree than the opposite if you put the code under my tree.
If nothing else, locality of code churn would say both this and IPoIB
belong here despite them being net drivers.

3) I would like some hard reasons why this driver deserves to exist?
I'm struggling very hard right now with why we would add an entirely new
"encapsulate IP over RDMA" driver.  Even if you use regular Ethernet
MACs instead of IPoIB's 20byte MAC, I'm struggling for why IPoIB
couldn't be modified to know it supports two MAC sizes and provide
different net devices based on those different types?  I'm struggling to
see why IPoIB couldn't be modified to essentially have two transport
layers underneath?  I haven't done a thorough code review yet, but if I
get into the net driver portion of this and it has very much similarity
to the IPoIB net portion, I'm probably going to want answers about why
this can't be a modification of IPoIB to support multiple
transport/encapsulation types instead of a separate driver even more.

-- 
Doug Ledford <dledford@...hat.com>
    GPG Key ID: 0E572FDD



Download attachment "signature.asc" of type "application/pgp-signature" (885 bytes)

Powered by blists - more mailing lists