[PATCH v5 net-next 00/36] nvme-tcp receive and tarnsmit offloads

[Or Gerlitz]

On Fri, Aug 6, 2021 at 10:46 PM Sagi Grimberg <sagi at grimberg.me> wrote:
> On 8/4/21 6:51 AM, Or Gerlitz wrote:
> > On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg <sagi at grimberg.me> wrote:

> >> [.. ] It is difficult to review.
> >> The order should be:
> >> 1. ulp_ddp interface
> >> 2. nvme-tcp changes
> >> 3. mlx5e changes
> >
> > .. and this is exactly how the series is organized, for v6 we will drop the
> > TX offload part and stick to completing the review on the RX offload part.
> >
> >> Also even beyond grouping patches together I have 2 requests:
> >> 1. Please consolidate ddp routines under a single ifdef (also minimize
> >> the ifdef in call-sites).
> >
> > ok, will make an effort to be better in that respect
> >
> >> 2. When consolidating functions, try to do this as prep patches
> >> documenting in the change log that it is preparing to add ddp. Its
> >> difficult digesting both at times.
> >
> > to clarify, you would like patch #5 "nvme-tcp: Add DDP offload control path"
> > to only add the call sites and if-not-deffed implementation for the added knobs:
> >
> > nvme_tcp_offload_socket
> > nvme_tcp_unoffload_socket
> > nvme_tcp_offload_limits
> > nvme_tcp_resync_response
> >
> > and a 2nd patch to add the if-yes-deffed implementation?
> >
> > This makes sense, however IMHO repeating this prep exercise for
> > the data-path patch (#6 "nvme-tcp: Add DDP data-path") doesn't
> > seem to provide notable value  b/c you will only see two call sites
> > for the two added empty knobs:
> >
> > nvme_tcp_setup_ddp
> > nvme_tcp_teardown_ddp
> >
> > but whatever you prefer, so.. let us know

> I was more referring to routines that now grew the ddp path
> and changed in the same time like:

> nvme_tcp_complete_request

not sure to follow on this one.. It's added on patch #6 "nvme-tcp: Add
DDP data-path"
and then used twice in the same patch replacing calls to nvme_try_complete_req
and then to nvme_complete_rq -- so how want this  be broken to prep/usage?

> nvme_tcp_consume_skb

this routine was born due to the ddp_ prefix addition to the iov copy
iter functions, which we are now removing due to feedback from Al

> etc..