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

From: Or Gerlitz <gerlitz.or@gmail.com>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: Christoph Hellwig <hch@lst.de>,
	Boris Pismenny <borisp@nvidia.com>,
	David Ahern <dsahern@gmail.com>, Jakub Kicinski <kuba@kernel.org>,
	David Miller <davem@davemloft.net>,
	Saeed Mahameed <saeedm@nvidia.com>,
	axboe@fb.com, Keith Busch <kbusch@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Eric Dumazet <edumazet@google.com>,
	Shai Malin <smalin@marvell.com>,
	boris.pismenny@gmail.com, linux-nvme@lists.infradead.org,
	Linux Netdev List <netdev@vger.kernel.org>,
	benishay@nvidia.com, Or Gerlitz <ogerlitz@nvidia.com>,
	Yoray Zack <yorayz@nvidia.com>
Subject: Re: [PATCH v5 net-next 00/36] nvme-tcp receive and tarnsmit offloads
Date: Tue, 10 Aug 2021 16:37:29 +0300	[thread overview]
Message-ID: <CAJ3xEMj=nLp4-GUSMYH0tgK69bfhuUYDjtE8fHFy=n2x2-cL_Q@mail.gmail.com> (raw)
In-Reply-To: <1d461a0d-cda5-2086-ec17-c5bb3a80846f@grimberg.me>

On Fri, Aug 6, 2021 at 10:46 PM Sagi Grimberg <sagi@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@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..