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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Aug 2021 16:37:29 +0300
From:   Or Gerlitz <gerlitz.or@...il.com>
To:     Sagi Grimberg <sagi@...mberg.me>
Cc:     Christoph Hellwig <hch@....de>, Boris Pismenny <borisp@...dia.com>,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...dia.com>, axboe@...com,
        Keith Busch <kbusch@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Eric Dumazet <edumazet@...gle.com>,
        Shai Malin <smalin@...vell.com>, boris.pismenny@...il.com,
        linux-nvme@...ts.infradead.org,
        Linux Netdev List <netdev@...r.kernel.org>,
        benishay@...dia.com, Or Gerlitz <ogerlitz@...dia.com>,
        Yoray Zack <yorayz@...dia.com>
Subject: Re: [PATCH v5 net-next 00/36] nvme-tcp receive and tarnsmit offloads

On Fri, Aug 6, 2021 at 10:46 PM Sagi Grimberg <sagi@...mberg.me> wrote:
> On 8/4/21 6:51 AM, Or Gerlitz wrote:
> > On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg <sagi@...mberg.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..