From: Kurt Kanzenbach <kurt@linutronix.de> To: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Sven Auhagen <sven.auhagen@voleatech.de>, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, bpf@vger.kernel.org, Ilias Apalodimas <ilias.apalodimas@linaro.org>, Lorenzo Bianconi <lorenzo@kernel.org>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Richard Cochran <richardcochran@gmail.com>, Alexander Duyck <alexander.duyck@gmail.com> Subject: Re: [PATCH net v3] igb: Fix XDP with PTP enabled Date: Fri, 23 Apr 2021 08:45:52 +0200 [thread overview] Message-ID: <878s59qz1b.fsf@kurt> (raw) In-Reply-To: <20210422101129.GB44289@ranger.igk.intel.com> [-- Attachment #1: Type: text/plain, Size: 2100 bytes --] On Thu Apr 22 2021, Maciej Fijalkowski wrote: > On Thu, Apr 22, 2021 at 07:26:17AM +0200, Kurt Kanzenbach wrote: >> + /* pull rx packet timestamp if available and valid */ >> + if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { >> + timestamp = igb_ptp_rx_pktstamp(rx_ring->q_vector, >> + pktbuf); >> + >> + if (timestamp) { >> + pkt_offset += IGB_TS_HDR_LEN; >> + size -= IGB_TS_HDR_LEN; >> + } >> + } > > Small nit: since this is a hot path, maybe we could omit the additional > branch that you're introducing above and make igb_ptp_rx_pktstamp() to > return either 0 for error cases and IGB_TS_HDR_LEN if timestamp was fine? > timestamp itself would be passed as an arg. > > So: > if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { > ts_offset = igb_ptp_rx_pktstamp(rx_ring->q_vector, > pktbuf, ×tamp); > pkt_offset += ts_offset; > size -= ts_offset; > } > > Thoughts? I feel like if we see that desc has timestamp enabled then let's > optimize it for successful case. Yes, this should work as well. Actually I didn't like the if statement either. Only one comment: It's not an offset but rather the timestamp header length. I'd call it 'ts_len'. > >> >> /* retrieve a buffer from the ring */ >> if (!skb) { >> - unsigned int offset = igb_rx_offset(rx_ring); >> - unsigned char *hard_start; >> + unsigned char *hard_start = pktbuf - igb_rx_offset(rx_ring); >> + unsigned int offset = pkt_offset + igb_rx_offset(rx_ring); > > Probably we could do something similar in flavour of: > https://lore.kernel.org/bpf/20210118151318.12324-10-maciej.fijalkowski@intel.com/ > > which broke XDP_REDIRECT and got fixed in: > https://lore.kernel.org/bpf/20210303153928.11764-2-maciej.fijalkowski@intel.com/ > > You get the idea. Yes, I do. However, I think such a change doesn't belong in this patch, which is a bugfix for XDP. It looks like an optimization. Should I split it into two patches and rather target net-next instead of net? Thanks for your review. Thanks, Kurt [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Kurt Kanzenbach <kurt@linutronix.de> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH net v3] igb: Fix XDP with PTP enabled Date: Fri, 23 Apr 2021 08:45:52 +0200 [thread overview] Message-ID: <878s59qz1b.fsf@kurt> (raw) In-Reply-To: <20210422101129.GB44289@ranger.igk.intel.com> On Thu Apr 22 2021, Maciej Fijalkowski wrote: > On Thu, Apr 22, 2021 at 07:26:17AM +0200, Kurt Kanzenbach wrote: >> + /* pull rx packet timestamp if available and valid */ >> + if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { >> + timestamp = igb_ptp_rx_pktstamp(rx_ring->q_vector, >> + pktbuf); >> + >> + if (timestamp) { >> + pkt_offset += IGB_TS_HDR_LEN; >> + size -= IGB_TS_HDR_LEN; >> + } >> + } > > Small nit: since this is a hot path, maybe we could omit the additional > branch that you're introducing above and make igb_ptp_rx_pktstamp() to > return either 0 for error cases and IGB_TS_HDR_LEN if timestamp was fine? > timestamp itself would be passed as an arg. > > So: > if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) { > ts_offset = igb_ptp_rx_pktstamp(rx_ring->q_vector, > pktbuf, ×tamp); > pkt_offset += ts_offset; > size -= ts_offset; > } > > Thoughts? I feel like if we see that desc has timestamp enabled then let's > optimize it for successful case. Yes, this should work as well. Actually I didn't like the if statement either. Only one comment: It's not an offset but rather the timestamp header length. I'd call it 'ts_len'. > >> >> /* retrieve a buffer from the ring */ >> if (!skb) { >> - unsigned int offset = igb_rx_offset(rx_ring); >> - unsigned char *hard_start; >> + unsigned char *hard_start = pktbuf - igb_rx_offset(rx_ring); >> + unsigned int offset = pkt_offset + igb_rx_offset(rx_ring); > > Probably we could do something similar in flavour of: > https://lore.kernel.org/bpf/20210118151318.12324-10-maciej.fijalkowski at intel.com/ > > which broke XDP_REDIRECT and got fixed in: > https://lore.kernel.org/bpf/20210303153928.11764-2-maciej.fijalkowski at intel.com/ > > You get the idea. Yes, I do. However, I think such a change doesn't belong in this patch, which is a bugfix for XDP. It looks like an optimization. Should I split it into two patches and rather target net-next instead of net? Thanks for your review. Thanks, Kurt -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 832 bytes Desc: not available URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20210423/94b8c793/attachment.asc>
next prev parent reply other threads:[~2021-04-23 6:46 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-22 5:26 [PATCH net v3] igb: Fix XDP with PTP enabled Kurt Kanzenbach 2021-04-22 5:26 ` [Intel-wired-lan] " Kurt Kanzenbach 2021-04-22 10:11 ` Maciej Fijalkowski 2021-04-22 10:11 ` [Intel-wired-lan] " Maciej Fijalkowski 2021-04-23 6:45 ` Kurt Kanzenbach [this message] 2021-04-23 6:45 ` Kurt Kanzenbach 2021-04-23 11:51 ` Maciej Fijalkowski 2021-04-23 11:51 ` [Intel-wired-lan] " Maciej Fijalkowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=878s59qz1b.fsf@kurt \ --to=kurt@linutronix.de \ --cc=alexander.duyck@gmail.com \ --cc=anthony.l.nguyen@intel.com \ --cc=ast@kernel.org \ --cc=bigeasy@linutronix.de \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=hawk@kernel.org \ --cc=ilias.apalodimas@linaro.org \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jesse.brandeburg@intel.com \ --cc=john.fastabend@gmail.com \ --cc=kuba@kernel.org \ --cc=lorenzo@kernel.org \ --cc=maciej.fijalkowski@intel.com \ --cc=netdev@vger.kernel.org \ --cc=richardcochran@gmail.com \ --cc=sven.auhagen@voleatech.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.