mbox series

[iwl-next,v4,0/4] igb: Add support for AF_XDP zero-copy

Message ID 20230804084051.14194-1-sriram.yagnaraman@est.tech (mailing list archive)
Headers show
Series igb: Add support for AF_XDP zero-copy | expand

Message

Sriram Yagnaraman Aug. 4, 2023, 8:40 a.m. UTC
The first couple of patches adds helper funcctions to prepare for AF_XDP
zero-copy support which comes in the last couple of patches, one each
for Rx and TX paths.

As mentioned in v1 patchset [0], I don't have access to an actual IGB
device to provide correct performance numbers. I have used Intel 82576EB
emulator in QEMU [1] to test the changes to IGB driver.

The tests use one isolated vCPU for RX/TX and one isolated vCPU for the
xdp-sock application [2]. Hope these measurements provide at the least
some indication on the increase in performance when using ZC, especially
in the TX path. It would be awesome if someone with a real IGB NIC can
test the patch.
 
AF_XDP performance using 64 byte packets in Kpps.
Benchmark:	XDP-SKB		XDP-DRV		XDP-DRV(ZC)
rxdrop		220		235		350
txpush		1.000		1.000		410
l2fwd 		1.000		1.000		200

AF_XDP performance using 1500 byte packets in Kpps.
Benchmark:	XDP-SKB		XDP-DRV		XDP-DRV(ZC)
rxdrop		200		210		310
txpush		1.000		1.000		410
l2fwd 		0.900		1.000		160

[0]: https://lore.kernel.org/intel-wired-lan/20230704095915.9750-1-sriram.yagnaraman@est.tech/
[1]: https://www.qemu.org/docs/master/system/devices/igb.html
[2]: https://github.com/xdp-project/bpf-examples/tree/master/AF_XDP-example

v3->v4:
- NULL check buffer_info in igb_dump before dereferencing (Simon Horman)

v2->v3:
- Avoid TX unit hang when using AF_XDP zero-copy by setting time_stamp
  on the tx_buffer_info
- Fix uninitialized nb_buffs (Simon Horman)

v1->v2:
- Use batch XSK APIs (Maciej Fijalkowski)
- Follow reverse xmas tree convention and remove the ternary operator
  use (Simon Horman)


Sriram Yagnaraman (4):
  igb: prepare for AF_XDP zero-copy support
  igb: Introduce XSK data structures and helpers
  igb: add AF_XDP zero-copy Rx support
  igb: add AF_XDP zero-copy Tx support

 drivers/net/ethernet/intel/igb/Makefile   |   2 +-
 drivers/net/ethernet/intel/igb/igb.h      |  35 +-
 drivers/net/ethernet/intel/igb/igb_main.c | 186 ++++++--
 drivers/net/ethernet/intel/igb/igb_xsk.c  | 522 ++++++++++++++++++++++
 4 files changed, 698 insertions(+), 47 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/igb/igb_xsk.c

Comments

Kurt Kanzenbach June 27, 2024, 7:07 a.m. UTC | #1
Hi Sriram,

On Fri Aug 04 2023, Sriram Yagnaraman wrote:
> The first couple of patches adds helper funcctions to prepare for AF_XDP
> zero-copy support which comes in the last couple of patches, one each
> for Rx and TX paths.
>
> As mentioned in v1 patchset [0], I don't have access to an actual IGB
> device to provide correct performance numbers. I have used Intel 82576EB
> emulator in QEMU [1] to test the changes to IGB driver.

I gave this patch series a try on a recent kernel and silicon
(i210). There was one issue in igb_xmit_zc(). But other than that it
worked very nicely.

It seems like it hasn't been merged yet. Do you have any plans for
continuing to work on this?

Thanks,
Kurt
Benjamin Steinke June 27, 2024, 4:49 p.m. UTC | #2
On Thursday, 27 June 2024, 09:07:55 CEST, Kurt Kanzenbach wrote:
> Hi Sriram,
> 
> On Fri Aug 04 2023, Sriram Yagnaraman wrote:
> > The first couple of patches adds helper funcctions to prepare for AF_XDP
> > zero-copy support which comes in the last couple of patches, one each
> > for Rx and TX paths.
> > 
> > As mentioned in v1 patchset [0], I don't have access to an actual IGB
> > device to provide correct performance numbers. I have used Intel 82576EB
> > emulator in QEMU [1] to test the changes to IGB driver.
> 
> I gave this patch series a try on a recent kernel and silicon
> (i210). There was one issue in igb_xmit_zc(). But other than that it
> worked very nicely.

Hi Kurt and Sriram,

I recently tried the patches on a 6.1 kernel. On two different devices i210 & 
i211 I couldn't see any packets being transmitted on the wire. Perhaps caused 
by the issue in igb_xmit_zc() you mentioned, Kurt? Can you share your findings, 
please?

RX seemed to work on first sight.

> It seems like it hasn't been merged yet. Do you have any plans for
> continuing to work on this?

I can offer to do testing and debugging on real hardware if this helps.

Thanks,
Benjamin
Kurt Kanzenbach June 27, 2024, 5:18 p.m. UTC | #3
Hi Benjamin,

On Thu Jun 27 2024, Benjamin Steinke wrote:
> On Thursday, 27 June 2024, 09:07:55 CEST, Kurt Kanzenbach wrote:
>> Hi Sriram,
>> 
>> On Fri Aug 04 2023, Sriram Yagnaraman wrote:
>> > The first couple of patches adds helper funcctions to prepare for AF_XDP
>> > zero-copy support which comes in the last couple of patches, one each
>> > for Rx and TX paths.
>> > 
>> > As mentioned in v1 patchset [0], I don't have access to an actual IGB
>> > device to provide correct performance numbers. I have used Intel 82576EB
>> > emulator in QEMU [1] to test the changes to IGB driver.
>> 
>> I gave this patch series a try on a recent kernel and silicon
>> (i210). There was one issue in igb_xmit_zc(). But other than that it
>> worked very nicely.
>
> Hi Kurt and Sriram,
>
> I recently tried the patches on a 6.1 kernel. On two different devices i210 & 
> i211 I couldn't see any packets being transmitted on the wire. Perhaps caused 
> by the issue in igb_xmit_zc() you mentioned, Kurt? Can you share your findings, 
> please?

Yeah, that's exactly the issue.

Following igb_xmit_xdp_ring() I've added PAYLEN to the Tx descriptor
instead of setting it to zero:

igb_xmit_zc()
{
        [...]

	/* put descriptor type bits */
	cmd_type = E1000_ADVTXD_DTYP_DATA | E1000_ADVTXD_DCMD_DEXT |
		   E1000_ADVTXD_DCMD_IFCS;
	olinfo_status = descs[i].len << E1000_ADVTXD_PAYLEN_SHIFT;
	
	cmd_type |= descs[i].len | IGB_TXD_DCMD;
	tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
	tx_desc->read.olinfo_status = cpu_to_le32(olinfo_status);

	[...]
}

Afterwards packets are transmitted on the wire.

>
> RX seemed to work on first sight.
>

Yes, Rx works even with PTP enabled.

>> It seems like it hasn't been merged yet. Do you have any plans for
>> continuing to work on this?
>
> I can offer to do testing and debugging on real hardware if this helps.

Great. Thanks!
Sriram Yagnaraman July 5, 2024, 9:22 p.m. UTC | #4
Hi,

> -----Original Message-----
> From: Kurt Kanzenbach <kurt@linutronix.de>
> Sent: Thursday, 27 June 2024 19:19
> To: Benjamin Steinke <benjamin.steinke@woks-audio.com>; Sriram
> Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: intel-wired-lan@osuosl.org; Maciej Fijalkowski
> <maciej.fijalkowski@intel.com>; Jesper Dangaard Brouer <hawk@kernel.org>;
> Daniel Borkmann <daniel@iogearbox.net>; netdev@vger.kernel.org;
> Jonathan Lemon <jonathan.lemon@gmail.com>; John Fastabend
> <john.fastabend@gmail.com>; Alexei Starovoitov <ast@kernel.org>; Björn
> Töpel <bjorn@kernel.org>; Eric Dumazet <edumazet@google.com>; Sriram
> Yagnaraman <sriram.yagnaraman@est.tech>; Tony Nguyen
> <anthony.l.nguyen@intel.com>; Jakub Kicinski <kuba@kernel.org>;
> bpf@vger.kernel.org; Paolo Abeni <pabeni@redhat.com>; David S . Miller
> <davem@davemloft.net>; Magnus Karlsson <magnus.karlsson@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v4 0/4] igb: Add support for
> AF_XDP zero-copy
> 
> Hi Benjamin,
> 
> On Thu Jun 27 2024, Benjamin Steinke wrote:
> > On Thursday, 27 June 2024, 09:07:55 CEST, Kurt Kanzenbach wrote:
> >> Hi Sriram,
> >>
> >> On Fri Aug 04 2023, Sriram Yagnaraman wrote:
> >> > The first couple of patches adds helper funcctions to prepare for
> >> > AF_XDP zero-copy support which comes in the last couple of patches,
> >> > one each for Rx and TX paths.
> >> >
> >> > As mentioned in v1 patchset [0], I don't have access to an actual
> >> > IGB device to provide correct performance numbers. I have used
> >> > Intel 82576EB emulator in QEMU [1] to test the changes to IGB driver.
> >>
> >> I gave this patch series a try on a recent kernel and silicon (i210).
> >> There was one issue in igb_xmit_zc(). But other than that it worked
> >> very nicely.
> >
> > Hi Kurt and Sriram,
> >
> > I recently tried the patches on a 6.1 kernel. On two different devices
> > i210 &
> > i211 I couldn't see any packets being transmitted on the wire. Perhaps
> > caused by the issue in igb_xmit_zc() you mentioned, Kurt? Can you
> > share your findings, please?
> 
> Yeah, that's exactly the issue.
> 
> Following igb_xmit_xdp_ring() I've added PAYLEN to the Tx descriptor instead
> of setting it to zero:
> 
> igb_xmit_zc()
> {
>         [...]
> 
> 	/* put descriptor type bits */
> 	cmd_type = E1000_ADVTXD_DTYP_DATA |
> E1000_ADVTXD_DCMD_DEXT |
> 		   E1000_ADVTXD_DCMD_IFCS;
> 	olinfo_status = descs[i].len << E1000_ADVTXD_PAYLEN_SHIFT;
> 
> 	cmd_type |= descs[i].len | IGB_TXD_DCMD;
> 	tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
> 	tx_desc->read.olinfo_status = cpu_to_le32(olinfo_status);
> 
> 	[...]
> }
> 
> Afterwards packets are transmitted on the wire.
> 
> >
> > RX seemed to work on first sight.
> >
> 
> Yes, Rx works even with PTP enabled.
> 
> >> It seems like it hasn't been merged yet. Do you have any plans for
> >> continuing to work on this?
> >
> > I can offer to do testing and debugging on real hardware if this helps.
> 
> Great. Thanks!

I have since changed my position at my company, and my new position doesn't allow me to contribute upstream to kernel unfortunately.
It would be great if one of you can take over this and get it delivered if possible.

Glad that others find this patch useful as well.
Kurt Kanzenbach July 8, 2024, 6:51 a.m. UTC | #5
On Fri Jul 05 2024, Sriram Yagnaraman wrote:
>> >> It seems like it hasn't been merged yet. Do you have any plans for
>> >> continuing to work on this?
>> >
>> > I can offer to do testing and debugging on real hardware if this helps.
>> 
>> Great. Thanks!
>
> I have since changed my position at my company, and my new position
> doesn't allow me to contribute upstream to kernel unfortunately.  It
> would be great if one of you can take over this and get it delivered
> if possible.

Ok, I'll take it over.

>
> Glad that others find this patch useful as well. 

Yeah, it's very useful :).

Thanks,
Kurt
Benjamin Steinke July 15, 2024, 11:34 a.m. UTC | #6
On Thursday, 27 June 2024, 19:18:37 CEST, Kurt Kanzenbach wrote:
> Hi Benjamin,
> 
> On Thu Jun 27 2024, Benjamin Steinke wrote:
> > On Thursday, 27 June 2024, 09:07:55 CEST, Kurt Kanzenbach wrote:
> >> Hi Sriram,
> >> 
> >> On Fri Aug 04 2023, Sriram Yagnaraman wrote:
> >> > The first couple of patches adds helper funcctions to prepare for
> >> > AF_XDP
> >> > zero-copy support which comes in the last couple of patches, one each
> >> > for Rx and TX paths.
> >> > 
> >> > As mentioned in v1 patchset [0], I don't have access to an actual IGB
> >> > device to provide correct performance numbers. I have used Intel
> >> > 82576EB
> >> > emulator in QEMU [1] to test the changes to IGB driver.
> >> 
> >> I gave this patch series a try on a recent kernel and silicon
> >> (i210). There was one issue in igb_xmit_zc(). But other than that it
> >> worked very nicely.
> > 
> > Hi Kurt and Sriram,
> > 
> > I recently tried the patches on a 6.1 kernel. On two different devices
> > i210 & i211 I couldn't see any packets being transmitted on the wire.
> > Perhaps caused by the issue in igb_xmit_zc() you mentioned, Kurt? Can you
> > share your findings, please?
> 
> Yeah, that's exactly the issue.
> 
> Following igb_xmit_xdp_ring() I've added PAYLEN to the Tx descriptor
> instead of setting it to zero:
> 
> igb_xmit_zc()
> {
>         [...]
> 
> 	/* put descriptor type bits */
> 	cmd_type = E1000_ADVTXD_DTYP_DATA | E1000_ADVTXD_DCMD_DEXT |
> 		   E1000_ADVTXD_DCMD_IFCS;
> 	olinfo_status = descs[i].len << E1000_ADVTXD_PAYLEN_SHIFT;
> 
> 	cmd_type |= descs[i].len | IGB_TXD_DCMD;
> 	tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
> 	tx_desc->read.olinfo_status = cpu_to_le32(olinfo_status);
> 
> 	[...]
> }
> 
> Afterwards packets are transmitted on the wire.

Hi Kurt,

I can confirm this makes the transmitter work.
Thank you for taking over this patch series and continue to bring this 
upstream. I will continue testing on this.
 
> > RX seemed to work on first sight.
> 
> Yes, Rx works even with PTP enabled.

I can confirm this as well. 

Best regards,
Benjamin