netdev - Re: [next-queue v2 2/4] i40e: Record number TXes cleaned during NAPI

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221006225656.GA86976@fastly.com>
Date:   Thu, 6 Oct 2022 15:56:57 -0700
From:   Joe Damato <jdamato@...tly.com>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     "Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        kuba@...nel.org, davem@...emloft.net, anthony.l.nguyen@...el.com
Subject: Re: [next-queue v2 2/4] i40e: Record number TXes cleaned during NAPI

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

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.