On Fri, Aug 6, 2021 at 10:46 PM Sagi Grimberg [off-list ref] wrote:

On 8/4/21 6:51 AM, Or Gerlitz wrote:

quoted

On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg [off-list ref] wrote:

quoted

quoted

[.. ] 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.

quoted

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

quoted

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..

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme