> From: Or Gerlitz [mailto:gerlitz.or@xxxxxxxxx] > > On Tue, May 24, 2016 at 9:40 AM, Nicholas A. Bellinger > <nab@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote: > >> > On Tue, Apr 19, 2016 at 9:30 PM, Varun Prakash <varun@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
