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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 7 Oct 2022 10:08:14 +0200 From: Maciej Fijalkowski <maciej.fijalkowski@...el.com> To: Joe Damato <jdamato@...tly.com> CC: Jesse Brandeburg <jesse.brandeburg@...el.com>, "Samudrala, Sridhar" <sridhar.samudrala@...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: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