From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-27 07:12:08
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
the xdp code paths will recycle pages.
This patch includes the rcu_read_lock/rcu_read_unlock pair noted by
Brenden Blanco in another pending patch.
net/mlx4_en: protect ring->xdp_prog with rcu_read_lock
CC: William Tu <redacted>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/e1000/e1000.h | 1
drivers/net/ethernet/intel/e1000/e1000_main.c | 168 ++++++++++++++++++++++++-
2 files changed, 165 insertions(+), 4 deletions(-)
@@ -279,6 +279,7 @@ struct e1000_adapter {structe1000_rx_ring*rx_ring,intcleaned_count);structe1000_rx_ring*rx_ring;/* One per active queue */+structbpf_prog*prog;structnapi_structnapi;intnum_tx_queues;
@@ -3298,6 +3341,61 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,returnNETDEV_TX_OK;}+staticvoide1000_tx_map_rxpage(structe1000_tx_ring*tx_ring,+structe1000_rx_buffer*rx_buffer_info,+unsignedintlen)+{+structe1000_tx_buffer*buffer_info;+unsignedinti=tx_ring->next_to_use;++buffer_info=&tx_ring->buffer_info[i];++buffer_info->length=len;+buffer_info->time_stamp=jiffies;+buffer_info->mapped_as_page=false;+buffer_info->dma=rx_buffer_info->dma;+buffer_info->next_to_watch=i;++tx_ring->buffer_info[i].skb=NULL;+tx_ring->buffer_info[i].segs=1;+tx_ring->buffer_info[i].bytecount=len;+tx_ring->buffer_info[i].next_to_watch=i;+}++staticvoide1000_xmit_raw_frame(structe1000_rx_buffer*rx_buffer_info,+unsignedintlen,+structnet_device*netdev,+structe1000_adapter*adapter)+{+structnetdev_queue*txq=netdev_get_tx_queue(netdev,0);+structe1000_hw*hw=&adapter->hw;+structe1000_tx_ring*tx_ring;++if(len>E1000_MAX_DATA_PER_TXD)+return;++/* e1000 only support a single txq at the moment so the queue is being+*sharedwithstack.Tosupportthisrequireslockingtoensurethe+*stackandXPDarenotrunningatthesametime.Deviceswould+*multiplequeuesshouldallocateaseparatequeuespace.+*/+HARD_TX_LOCK(netdev,txq,smp_processor_id());++tx_ring=adapter->tx_ring;++if(E1000_DESC_UNUSED(tx_ring)<2)+return;++e1000_tx_map_rxpage(tx_ring,rx_buffer_info,len);++e1000_tx_queue(adapter,tx_ring,0/*tx_flags*/,1);++writel(tx_ring->next_to_use,hw->hw_addr+tx_ring->tdt);+mmiowb();++HARD_TX_UNLOCK(netdev,txq);+}+#define NUM_REGS 38 /* 1 based count */staticvoide1000_regdump(structe1000_adapter*adapter){
From: Or Gerlitz <hidden> Date: 2016-08-28 05:55:56
On Sat, Aug 27, 2016 at 10:11 AM, John Fastabend
[off-list ref] wrote:
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
the xdp code paths will recycle pages.
nit, better to avoid random cleanups in a patch adding new (&& cool)
functionality
cleaned = true;
cleaned_count++;
+ length = le16_to_cpu(rx_desc->length);
+
+ if (prog) {
+ struct page *p = buffer_info->rxbuf.page;
+ dma_addr_t dma = buffer_info->dma;
+ int act;
+
+ if (unlikely(!(status & E1000_RXD_STAT_EOP))) {
+ /* attached bpf disallows larger than page
+ * packets, so this is hw error or corruption
+ */
+ pr_info_once("%s buggy !eop\n", netdev->name);
+ break;
+ }
+ if (unlikely(rx_ring->rx_skb_top)) {
+ pr_info_once("%s ring resizing bug\n",
+ netdev->name);
+ break;
+ }
+ dma_sync_single_for_cpu(&pdev->dev, dma,
+ length, DMA_FROM_DEVICE);
+ act = e1000_call_bpf(prog, page_address(p), length);
+ switch (act) {
+ case XDP_PASS:
+ break;
+ case XDP_TX:
+ dma_sync_single_for_device(&pdev->dev,
+ dma,
+ length,
+ DMA_TO_DEVICE);
+ e1000_xmit_raw_frame(buffer_info, length,
+ netdev, adapter);
+ /* Fallthrough to re-use mappedg page after xmit */
Did you want to say "mapped"? wasn't sure what's the role of "g" @ the end
+ case XDP_DROP:
+ default:
+ /* re-use mapped page. keep buffer_info->dma
+ * as-is, so that e1000_alloc_jumbo_rx_buffers
+ * only needs to put it back into rx ring
+ */
if we're on the XDP_TX pass, don't we need to actually see that frame
has been xmitted
before re using the page?
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-08-28 12:23:28
On 16-08-27 03:11 AM, John Fastabend wrote:
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
the xdp code paths will recycle pages.
This patch includes the rcu_read_lock/rcu_read_unlock pair noted by
Brenden Blanco in another pending patch.
net/mlx4_en: protect ring->xdp_prog with rcu_read_lock
Do you have any perf numbers of drops of this vs tc drop at ingress?
single or multiple cpus.
cheers,
jamal
From: William Tu <hidden> Date: 2016-08-28 15:57:35
Hi,
Reading through the patch, I found some minor typos below.
On Sat, Aug 27, 2016 at 12:11 AM, John Fastabend
[off-list ref] wrote:
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
I think you mean XDP_PASS instead of XDP_XMIT?
the xdp code paths will recycle pages.
This patch includes the rcu_read_lock/rcu_read_unlock pair noted by
Brenden Blanco in another pending patch.
net/mlx4_en: protect ring->xdp_prog with rcu_read_lock
CC: William Tu <redacted>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/e1000/e1000.h | 1
drivers/net/ethernet/intel/e1000/e1000_main.c | 168 ++++++++++++++++++++++++-
2 files changed, 165 insertions(+), 4 deletions(-)
+static void e1000_xmit_raw_frame(struct e1000_rx_buffer *rx_buffer_info,
+ unsigned int len,
+ struct net_device *netdev,
+ struct e1000_adapter *adapter)
+{
+ struct netdev_queue *txq = netdev_get_tx_queue(netdev, 0);
+ struct e1000_hw *hw = &adapter->hw;
+ struct e1000_tx_ring *tx_ring;
+
+ if (len > E1000_MAX_DATA_PER_TXD)
+ return;
+
+ /* e1000 only support a single txq at the moment so the queue is being
+ * shared with stack. To support this requires locking to ensure the
+ * stack and XPD are not running at the same time. Devices would
+ * multiple queues should allocate a separate queue space.
+ */
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-29 05:34:12
On 16-08-27 10:55 PM, Or Gerlitz wrote:
On Sat, Aug 27, 2016 at 10:11 AM, John Fastabend
[off-list ref] wrote:
quoted
From: Alexei Starovoitov <redacted>
quoted
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
the xdp code paths will recycle pages.
nit, better to avoid random cleanups in a patch adding new (&& cool)
functionality
Yep thanks.
[...]
quoted
+ case XDP_TX:
+ dma_sync_single_for_device(&pdev->dev,
+ dma,
+ length,
+ DMA_TO_DEVICE);
+ e1000_xmit_raw_frame(buffer_info, length,
+ netdev, adapter);
+ /* Fallthrough to re-use mappedg page after xmit */
Did you want to say "mapped"? wasn't sure what's the role of "g" @ the end
Yep but see below...
quoted
+ case XDP_DROP:
+ default:
+ /* re-use mapped page. keep buffer_info->dma
+ * as-is, so that e1000_alloc_jumbo_rx_buffers
+ * only needs to put it back into rx ring
+ */
if we're on the XDP_TX pass, don't we need to actually see that frame
has been xmitted
before re using the page?
Agreed this seems to be too ambitious in the XDP_TX case. Thanks for
the help. Unless Alexei has some reason why it works I'll go ahead and
consume the buffer here.
I think setting
+ bi->rxbuf.page = NULL;
at the end of the XDP_TX case should fix it but I'll test it again,
Thanks again I guess this is what I get for trying to push patches out
on Friday night.
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-08-29 05:37:16
On 16-08-28 08:56 AM, William Tu wrote:
Hi,
Reading through the patch, I found some minor typos below.
On Sat, Aug 27, 2016 at 12:11 AM, John Fastabend
[off-list ref] wrote:
quoted
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
I think you mean XDP_PASS instead of XDP_XMIT?
I really meant XDP_TX but see Or's note and next revision will have
XDP_DROP only here.
quoted
the xdp code paths will recycle pages.
This patch includes the rcu_read_lock/rcu_read_unlock pair noted by
Brenden Blanco in another pending patch.
net/mlx4_en: protect ring->xdp_prog with rcu_read_lock
CC: William Tu <redacted>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: John Fastabend <redacted>
---
drivers/net/ethernet/intel/e1000/e1000.h | 1
drivers/net/ethernet/intel/e1000/e1000_main.c | 168 ++++++++++++++++++++++++-
2 files changed, 165 insertions(+), 4 deletions(-)
+static void e1000_xmit_raw_frame(struct e1000_rx_buffer *rx_buffer_info,
+ unsigned int len,
+ struct net_device *netdev,
+ struct e1000_adapter *adapter)
+{
+ struct netdev_queue *txq = netdev_get_tx_queue(netdev, 0);
+ struct e1000_hw *hw = &adapter->hw;
+ struct e1000_tx_ring *tx_ring;
+
+ if (len > E1000_MAX_DATA_PER_TXD)
+ return;
+
+ /* e1000 only support a single txq at the moment so the queue is being
+ * shared with stack. To support this requires locking to ensure the
+ * stack and XPD are not running at the same time. Devices would
+ * multiple queues should allocate a separate queue space.
+ */
On Sun, 28 Aug 2016 08:23:26 -0400
Jamal Hadi Salim [off-list ref] wrote:
On 16-08-27 03:11 AM, John Fastabend wrote:
quoted
From: Alexei Starovoitov <redacted>
This patch adds initial support for XDP on e1000 driver. Note e1000
driver does not support page recycling in general which could be
added as a further improvement. However for XDP_DROP and XDP_XMIT
the xdp code paths will recycle pages.
This patch includes the rcu_read_lock/rcu_read_unlock pair noted by
Brenden Blanco in another pending patch.
net/mlx4_en: protect ring->xdp_prog with rcu_read_lock
Do you have any perf numbers of drops of this vs tc drop at ingress?
Hi Jamal,
Can you please provide a simple "tc" command that implements "tc drop"?
Then, I'll add this to the series of tests I'm using for (what I call)
"zoom-in" benchmarking.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-08-29 10:53:56
On 16-08-29 04:30 AM, Jesper Dangaard Brouer wrote:
Hi Jamal,
Can you please provide a simple "tc" command that implements "tc drop"?
Then, I'll add this to the series of tests I'm using for (what I call)
"zoom-in" benchmarking.
Thanks Jesper.
Something simple since this is done in ingress; lets say drop icmp
packets:
export ETH=eth0
export TC=/sbin/tc
#delete existing ingress qdisc - flushes all filters/actions
sudo $TC qdisc del dev $ETH ingress
#re-add ingress
sudo $TC qdisc add dev $ETH ingress
#
#simple rule to drop all icmp
sudo $TC filter add dev $ETH parent ffff: prio 4 protocol ip \
u32 match ip protocol 1 0xff flowid 1:1 \
action drop
# other type of filters if you want to compare instead of above
#
# a)drop all
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match u32 0 0 flowid 1:1 \
action drop
#b) drop if src is XXX
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match ip src 192.168.100.1 flowid 1:1 \
action drop
If you can do one core vs many cores (XDP should probably do very well
in multi-core)
I think given how ancient the e1000 is we may see the driver being
a contributing overhead. I believe XDP given location will do
well - but for this kind of driver my gut feeling is probably not
by large margin.
cheers,
jamal
On Mon, 29 Aug 2016 06:53:53 -0400
Jamal Hadi Salim [off-list ref] wrote:
On 16-08-29 04:30 AM, Jesper Dangaard Brouer wrote:
quoted
Hi Jamal,
Can you please provide a simple "tc" command that implements "tc drop"?
Then, I'll add this to the series of tests I'm using for (what I call)
"zoom-in" benchmarking.
Thanks Jesper.
I've created a script called tc_ingress_drop.sh[1] which uses the
commands you provided below. Now people can easily use this script to
perform the benchmark you were requesting ;-)
[1] https://github.com/netoptimizer/network-testing/blob/master/bin/tc_ingress_drop.sh
Example to enable dropping:
$ ./tc_ingress_drop.sh --dev mlx5p2 --verbose
# (Not root, running with sudo)
# Flush existing ingress qdisc on device :mlx5p2
tc qdisc del dev mlx5p2 ingress
tc qdisc add dev mlx5p2 ingress
# Simply drop all ingress packets on device: mlx5p2
tc filter add dev mlx5p2 parent ffff: prio 2 protocol ip u32 match u32 0 0 flowid 1:1 action drop
Example to disable again:
./tc_ingress_drop.sh --dev mlx5p2 --flush
Something simple since this is done in ingress; lets say drop icmp
packets:
export ETH=eth0
export TC=/sbin/tc
#delete existing ingress qdisc - flushes all filters/actions
sudo $TC qdisc del dev $ETH ingress
#re-add ingress
sudo $TC qdisc add dev $ETH ingress
#
#simple rule to drop all icmp
sudo $TC filter add dev $ETH parent ffff: prio 4 protocol ip \
u32 match ip protocol 1 0xff flowid 1:1 \
action drop
# other type of filters if you want to compare instead of above
#
# a)drop all
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match u32 0 0 flowid 1:1 \
action drop
#b) drop if src is XXX
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match ip src 192.168.100.1 flowid 1:1 \
action drop
Hi Jamal,
I'm adding: drop a specific UDP port option to my script... But I does
not match/drop the packets, command below does apply, but it does not
work in practice
$ ./tc_ingress_drop.sh --verbose --dev mlx5p2 --port 9
tc qdisc del dev mlx5p2 ingress
tc qdisc add dev mlx5p2 ingress
tc filter add dev mlx5p2 parent ffff: prio 4 protocol ip u32 match ip protocol 17 0xff match udp dst 9 0xffff flowid 1:1 action drop
(Use-case is obviously to drop pktgen UDP packets.)
I also tried with:
tc filter add dev mlx5p2 parent ffff: prio 4 protocol ip \
u32 \
match udp dst 9 0xffff \
match ip protocol 17 0xff flowid 1:1 action drop
--Jesper
(top post)
On Mon, 29 Aug 2016 15:39:05 +0200 Jesper Dangaard Brouer [off-list ref] wrote:
On Mon, 29 Aug 2016 06:53:53 -0400
Jamal Hadi Salim [off-list ref] wrote:
quoted
On 16-08-29 04:30 AM, Jesper Dangaard Brouer wrote:
quoted
Hi Jamal,
Can you please provide a simple "tc" command that implements "tc drop"?
Then, I'll add this to the series of tests I'm using for (what I call)
"zoom-in" benchmarking.
Thanks Jesper.
I've created a script called tc_ingress_drop.sh[1] which uses the
commands you provided below. Now people can easily use this script to
perform the benchmark you were requesting ;-)
[1] https://github.com/netoptimizer/network-testing/blob/master/bin/tc_ingress_drop.sh
Example to enable dropping:
$ ./tc_ingress_drop.sh --dev mlx5p2 --verbose
# (Not root, running with sudo)
# Flush existing ingress qdisc on device :mlx5p2
tc qdisc del dev mlx5p2 ingress
tc qdisc add dev mlx5p2 ingress
# Simply drop all ingress packets on device: mlx5p2
tc filter add dev mlx5p2 parent ffff: prio 2 protocol ip u32 match u32 0 0 flowid 1:1 action drop
Example to disable again:
./tc_ingress_drop.sh --dev mlx5p2 --flush
quoted
Something simple since this is done in ingress; lets say drop icmp
packets:
export ETH=eth0
export TC=/sbin/tc
#delete existing ingress qdisc - flushes all filters/actions
sudo $TC qdisc del dev $ETH ingress
#re-add ingress
sudo $TC qdisc add dev $ETH ingress
#
#simple rule to drop all icmp
sudo $TC filter add dev $ETH parent ffff: prio 4 protocol ip \
u32 match ip protocol 1 0xff flowid 1:1 \
action drop
# other type of filters if you want to compare instead of above
#
# a)drop all
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match u32 0 0 flowid 1:1 \
action drop
#b) drop if src is XXX
sudo $TC filter add dev $ETH parent ffff: prio 2 protocol ip \
u32 match ip src 192.168.100.1 flowid 1:1 \
action drop
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-08-30 12:13:18
On 16-08-29 11:55 AM, Jesper Dangaard Brouer wrote:
tc filter add dev mlx5p2 parent ffff: prio 4 protocol ip u32 match ip protocol 17 0xff match udp dst 9 0xffff flowid 1:1 action
Syntax is a little more convoluted than that ;->. Try:
sudo tc filter add dev eth0 parent ffff: prio 4 protocol ip u32 \
match ip protocol 17 0xff \
match ip dport 1900 0xffff \
flowid 1:1 \
action drop
Note, this will be more cycles than drop all.
cheers,
jamal
On Tue, 30 Aug 2016 08:13:15 -0400 Jamal Hadi Salim [off-list ref] wrote:
On 16-08-29 11:55 AM, Jesper Dangaard Brouer wrote:
quoted
tc filter add dev mlx5p2 parent ffff: prio 4 protocol ip u32 match ip protocol 17 0xff match udp dst 9 0xffff flowid 1:1 action
Syntax is a little more convoluted than that ;->. Try:
sudo tc filter add dev eth0 parent ffff: prio 4 protocol ip u32 \
match ip protocol 17 0xff \
match ip dport 1900 0xffff \
flowid 1:1 \
action drop
I think I figured out why, match "udp dst" does not work. It seems to
depend on "nexthdr+0" which is an implicit variable, that for unknown
reasons are not set in my original rule (above).
Before you suggestion I managed to match the udp port by manually
defining the offset, assuming an IP-header is 20 bytes (no-options),
like:
tc filter add dev $device parent ffff: prio 4 protocol ip \
u32 \
match ip protocol 17 0xff \
match udp dst $udp_port 0xffff at 21\
flowid 1:1 \
action drop
You solution with "ip dport" also works, but man[1] tc-u32(8) also have
a warning about "ip dport" size assumptions...
Updated my script to use "u32 match ip port":
https://github.com/netoptimizer/network-testing/commit/6449f6beb4d2
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-09-01 21:19:30
[...]
I think given how ancient the e1000 is we may see the driver being
a contributing overhead. I believe XDP given location will do
well - but for this kind of driver my gut feeling is probably not
by large margin.
Right so just ran the baseline, xdp, tc spread and its all more or
less in the noise where you drop the packets doesn't matter much. At
least in my setup where I'm running e1000 in a VM backed by a tap
device. I don't have a physical e1000 in my system at the moment to
test.
I still think this code is valuable though because it lets me run
the same XDP program in a e1000 VM that I'm running on my 10/40Gbps
NIC. Also it gives me a nice test platform to work on.
I'm going to resubmit the patch with a couple of the fixes pointed out
by others.
Thanks,
John
From: John Fastabend <john.fastabend@gmail.com> Date: 2016-09-01 21:35:37
On 16-08-30 06:31 AM, Jesper Dangaard Brouer wrote:
On Tue, 30 Aug 2016 08:13:15 -0400 Jamal Hadi Salim [off-list ref] wrote:
quoted
On 16-08-29 11:55 AM, Jesper Dangaard Brouer wrote:
quoted
tc filter add dev mlx5p2 parent ffff: prio 4 protocol ip u32 match ip protocol 17 0xff match udp dst 9 0xffff flowid 1:1 action
Syntax is a little more convoluted than that ;->. Try:
sudo tc filter add dev eth0 parent ffff: prio 4 protocol ip u32 \
match ip protocol 17 0xff \
match ip dport 1900 0xffff \
flowid 1:1 \
action drop
I think I figured out why, match "udp dst" does not work. It seems to
depend on "nexthdr+0" which is an implicit variable, that for unknown
reasons are not set in my original rule (above).
Before you suggestion I managed to match the udp port by manually
defining the offset, assuming an IP-header is 20 bytes (no-options),
like:
tc filter add dev $device parent ffff: prio 4 protocol ip \
u32 \
match ip protocol 17 0xff \
match udp dst $udp_port 0xffff at 21\
flowid 1:1 \
action drop
You solution with "ip dport" also works, but man[1] tc-u32(8) also have
a warning about "ip dport" size assumptions...
Updated my script to use "u32 match ip port":
https://github.com/netoptimizer/network-testing/commit/6449f6beb4d2
FWIW the 'udp dst' notation is quit fragile in that it only reads an
offset into the packet where a udp dst port might be. More robust
solutions require the use of links.
I have a wrapper tool around the 'link' creation part of u32 that we
can probably show off at netconf. :)
quoted
Note, this will be more cycles than drop all.
Yes, that is the point ;-) XDP also does header parsing...