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  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:   Fri, 7 Oct 2022 10:08:14 +0200
From:   Maciej Fijalkowski <>
To:     Joe Damato <>
CC:     Jesse Brandeburg <>,
        "Samudrala, Sridhar" <>,
        <>, <>,
        <>, <>,
Subject: Re: [next-queue v2 2/4] i40e: Record number TXes cleaned during NAPI

On Thu, Oct 06, 2022 at 03:56:57PM -0700, Joe Damato wrote:
> On Thu, Oct 06, 2022 at 03:35:36PM -0700, Jesse Brandeburg wrote:
> > On 10/6/2022 10:32 AM, Joe Damato wrote:
> > >Sorry, but I don't see the value in the second param. NAPI decides what to
> > >do based on nb_pkts. That's the only parameter that matters for the purpose
> > >of NAPI going into poll mode or not, right?
> > >
> > >If so: I don't see any reason why a second parameter is necessary.
> > 
> > Sridhar and I talked about this offline. We agree now that you can just
> > proceed with the single parameter.
> OK, thanks.
> > >
> > >As I mentioned earlier: if it's just that the name of the parameter isn't
> > >right (e.g., you want it to be 'tx_processed' instead of 'tx_cleaned') then
> > >that's an easy fix; I'll just change the name.
> > 
> > I think the name change isn't necessary, since we're not going to extend
> > this patch with full XDP events printed (see below)

So better to keep the twisted naming?

> > 
> > >
> > >It doesn't seem helpful to have xsk_frames as an out parameter for
> > >i40e_napi_poll tracepoint; that value is not used to determine anything
> > >about i40e's NAPI.
> > >
> > >>I am not completely clear on the reasoning behind setting clean_complete
> > >>based on number of packets transmitted in case of XDP.
> > >>>
> > >>>>That might reduce the complexity a bit, and will probably still be pretty
> > >>>>useful for people tuning their non-XDP workloads.
> > >>
> > >>This option is fine too.
> > >
> > >I'll give Jesse a chance to weigh in before I proceed with spinning a v3.
> > 
> > I'm ok with the patch you have now, that shows nb_pkts because it's the
> > input to the polling decision. We can add the detail about XDP transmits
> > cleaned in a later series or patch that is by someone who wants the XDP
> > details in the napi poll context.

Please spell out whole AF_XDP instead of referring to XDP. Future readers
might get confused. XDP is totally fine with what Joe is doing, I'm trying
to bring up whole AF_XDP term and I feel like I'm being ignored.

number of produced packets to HW tx ring != number of produced packets to
AF_XDP CQ ring.

> Thanks for the detailed and thoughtful feedback, it is much appreciated.
> I'll leave this patch the way it is then and tweak the RX patch to include
> an rx_clean_complete boolean as I mentioned in my response to that patch
> and send out a v3.
> FWIW, I had assumed that you would suggest dropping the XDP stuff so I
> pre-emptively spun a branch locally that dropped it... it is a much smaller
> change of course, but I suspect that this tracepoint might useful for XDP
> users, so I think the decision to leave it with nb_pkts makes sense.
> Thanks again for the review. I'll send a v3 shortly.

Powered by blists - more mailing lists