On Wed, 2016-05-25 at 23:41 +0300, Or Gerlitz wrote: > On Wed, May 25, 2016 at 8:40 PM, Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx> wrote: > >> From: Or Gerlitz [mailto:gerlitz.or@xxxxxxxxx] > >> <SNIP> > . > > > > 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. > > sounds good > > > 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. > > Ditto > > > 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. > > Well, I am not sure. It's like saying that if you break the IB/RoCE CM > to active side and passive side they will not share lots of code. If > you look on the existing CM (drivers/infiniband/core/cm.c) and think > how to break it to two, you'll see that you end up with two code bases > to maintain with likelihood of same bugs and similar state machines to > implement/maintain and debug. So in that sense, it's much (much) > better to have one CM code which does both iscsi sides. > > > 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(). > > good and one code base which treats both sides. > > > 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? > > We've been there, e.g Intel recently sent iWARP driver and throughout > the review we realized that lots of the iwarp netlink code is shared > between existing drivers and the new driver, so the driver didn't get > in kernel X but rather X+1 after doing that fix, it's only off by > one... > > I don't think we should be letting duplication of that extent to get > in, for Chelsio ppl that know the materials well better vs anyone else > it should have been clear that they create that dup without any real > justification and till that moment they didn't come here to even > comment on that. Lets have them fix that for the next merge window, > that's my thinking, Nic? > Varun + Co have made common improvements between existing software iscsi-target RX + TX PDU handling and their new driver, and no further comments for these prerequisites have been made. The additional improvements discussed here so far are cxgb* specific, and will not effect other drivers, and will not change configfs user ABI layout compatibility within /sys/kernel/config/target/iscsi/. That being the case, I think it's a reasonable starting point for mainline users to consume target ISO on cxgb hardware, and for Chelsio to make further common code improvements across their existing host drivers. -- 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
