RE: [PATCH v3 13/13] cxgbit: add files for cxgbit.ko

["Steve Wise" <swise@opengridcomputing.com>]

> From: Or Gerlitz [mailto:gerlitz.or@gmail.com]
> 
> On Tue, May 24, 2016 at 9:40 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > Hi Or & Co,
> > On Wed, 2016-05-18 at 14:45 +0300, Or Gerlitz wrote:
> >> On Sat, Apr 30, 2016 at 6:54 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> >> > On Tue, Apr 19, 2016 at 9:30 PM, Varun Prakash <varun@chelsio.com> wrote:
> >> >> cxgbit.h - This file contains data structure
> >> >> definitions for cxgbit.ko.
> >> >>
> >> >> cxgbit_lro.h - This file contains data structure
> >> >> definitions for LRO support.
> >> >>
> >> >> cxgbit_main.c - This file contains code for
> >> >> registering with iscsi target transport and
> >> >> cxgb4 driver.
> >> >>
> >> >> cxgbit_cm.c - This file contains code for
> >> >> connection management.
> >> >>
> >> >> cxgbit_target.c - This file contains code
> >> >> for processing iSCSI PDU.
> >> >>
> >> >> cxgbit_ddp.c - This file contains code for
> >> >> Direct Data Placement.
> >> >
> >> > Wait,
> >> >
> >> > You are adding many K's LOCs to handle things like CM (connection
> >> > management), DDP and LRO. But your upstream solution must be using CM
> >> > and DDP (and LRO as well) for the HW offloaded initiator side as well,
> >> > not to mention the iWARP side of things.
> >> >
> >> > There must be some way to refactor things instead of repeating the
> >> > same bits over and over, thoughts?
> 
> >> The author haven't responded... where that this stands from your point of view?
> 
> > For an initial merge, I don't have an objection to this series wrt
> > drivers/target/iscsi/* improvements + prerequisites, and new standalone
> > cxgbit iscsit_transport driver.
> >
> > That said, there are areas between cxgbi + cxgbit code that can be made
> > common as you've pointed out.  The Cheliso folks have mentioned off-list
> > that cxgbi as-is in mainline does not support LRO, and that the majority
> > of DDP logic is shared between initiator + target.
> >
> > Are there specific pieces of logic in DDP or iWARP for cxgb* that you'd
> > like to see Varun + Co pursue as common code in v4.8+..?
> 
> Hi Nic,
> 
> As I wrote above, I have good reasons to believe that there are few K
> LOCs of duplication
> between this series to the chelsio hw iscsi initiator or the chelsio
> iwarp driver or both (triple).
> 
> Ys, I'd like to see a public response from Varun and Co on this valid
> reviewer comment
> before you proceed with this series, makes sense? There's no point to
> duplicate the same
> code in the kernel again and again. **Even** if there's one
> duplication now, we don't want
> another one.
> 
> Or.

Hey Or and Nicholas,

I've been staying out of this directly due to my workload.  But I will now 
take a look at where and how we can refactor to reduce the code duplication. 
Here are some thoughts with a quick glance:

DDP is not used by iWARP.  DDP is for direct placement attempt on a TCP 
streaming mode connection.  RDMA has its own way of doing this that doesn’t 
utilize the same HW resources.  So I would say that the DDP usage could be 
refactored such that there is a set of helper functions to do the common 
logic, like managing page pod adapter resources.  An example would be 
cxgb4i.c:ddp_ppod_write_idata(), and cxgbit_ppod_write_idata() from patch 
13.

Connection Management (CM) can definitely be refactored to share helper 
functions for setting up/tearing down TCP connections.  But the iWARP logic 
is very different from iscsi once the TCP connection is setup and the 
connection moved into RDMA mode.  So the sharing of logic would be in 
sending various CPL messages to the adapter to connect/accept/close/abort 
connections.  Having said that, the initiator and target wouldn't share a 
lot by virtue of the initiator only doing active side connection setup, and 
the target only doing passive side connection setup.  So refactor would be 
common services that iw_cxgb4, cxgbi4, and cxgbit all use.    An example 
would be: iw_cxgb4/cm.c:send_connect(), and cxgb4i.c/send_act_open_req().

I didn't look at LRO at this point.

Anyway, none of these are particularly difficult, but will require 
significant effort and time.  The current cxgbit series has had a lot of 
internal review and testing by the Chelsio iscsi team, and it would be great 
if this refactoring can be deferred with the promise that we will get it 
done for the 4.8 merge window.  Thoughts?

Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html