From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:24
Hi!
This set adds XDP flag for forcing offload and a attachement mode
for reporting to user space that program has been offloaded. The
nfp driver is modified to make use of the new flags, but also to
adhere to the DRV_MODE flag which should disable the HW offload.
Note that the NFP driver currently claims XDP offload support but
lacks most basic features like direct packet access.
Jakub Kicinski (8):
xdp: pass XDP flags into install handlers
xdp: add HW offload mode flag for installing programs
nfp: xdp: move driver XDP setup into a separate function
nfp: bpf: don't offload XDP programs in DRV_MODE
nfp: bpf: take a reference on offloaded programs
nfp: bpf: add support for XDP_FLAGS_HW_MODE
xdp: add reporting of offload mode
nfp: xdp: report if program is offloaded
drivers/net/ethernet/netronome/nfp/nfp_net.h | 5 ++
.../net/ethernet/netronome/nfp/nfp_net_common.c | 53 ++++++++++++++++------
include/linux/netdevice.h | 8 ++--
include/uapi/linux/if_link.h | 8 +++-
net/core/dev.c | 10 ++--
net/core/rtnetlink.c | 10 ++--
6 files changed, 66 insertions(+), 28 deletions(-)
--
2.11.0
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:25
Pass XDP flags to the xdp ndo. This will allow drivers to look
at the mode flags and make decisions about offload.
Signed-off-by: Jakub Kicinski <redacted>
---
include/linux/netdevice.h | 1 +
net/core/dev.c | 5 +++--
2 files changed, 4 insertions(+), 2 deletions(-)
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:25
Add an installation-time flag for requesting that the program
be installed only if it can be offloaded to HW.
Internally new command for ndo_xdp is added, this way we avoid
putting checks into drivers since they all return -EINVAL on
an unknown command.
Signed-off-by: Jakub Kicinski <redacted>
---
include/linux/netdevice.h | 1 +
include/uapi/linux/if_link.h | 7 +++++--
net/core/dev.c | 7 +++++--
net/core/rtnetlink.c | 4 ++--
4 files changed, 13 insertions(+), 6 deletions(-)
@@ -807,6 +807,7 @@ enum xdp_netdev_command {*whenitisnolongerused.*/XDP_SETUP_PROG,+XDP_SETUP_PROG_HW,/* Check if a bpf program is set on the device. The callee should*returntrueifaprogramiscurrentlyattachedandrunning.*/
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:27
In preparation of XDP offload flags move the driver setup into
a function. Otherwise the number of conditions in one function
would make it slightly hard to follow. The offload handler may
now be called with NULL prog, even if no offload is currently
active, but that's fine, offload code can handle that.
Signed-off-by: Jakub Kicinski <redacted>
---
.../net/ethernet/netronome/nfp/nfp_net_common.c | 23 +++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -3300,13 +3300,26 @@ static int nfp_net_xdp_setup(struct nfp_net *nn, struct netdev_xdp *xdp)dp->rx_dma_off=prog?XDP_PACKET_HEADROOM-nn->dp.rx_offset:0;/* We need RX reconfig to remap the buffers (BIDIR vs FROM_DEV) */-err=nfp_net_ring_reconfig(nn,dp,xdp->extack);+err=nfp_net_ring_reconfig(nn,dp,extack);if(err)returnerr;if(old_prog)bpf_prog_put(old_prog);+return0;+}++staticint+nfp_net_xdp_setup(structnfp_net*nn,structbpf_prog*prog,+structnetlink_ext_ack*extack)+{+interr;++err=nfp_net_xdp_setup_drv(nn,prog,extack);+if(err)+returnerr;+nfp_app_xdp_offload(nn->app,nn,nn->dp.xdp_prog);return0;
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:27
DRV_MODE means that user space wants the program to be run in
the driver. Do not try to offload. Only offload if no mode
flags have been specified.
Remember what the mode is when the program is installed and refuse
new setup requests if there is already a program loaded in a
different mode. This should leave it open for us to implement
simultaneous loading of two programs - one in the drv path and
another to the NIC later.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 +++
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 14 +++++++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:28
The xdp_prog member of the adapter's data path structure is used
for XDP in driver mode. In case a XDP program is loaded with in
HW-only mode, we need to store it somewhere else. Add a new XDP
prog pointer in the main structure and use that when we need to
know whether any XDP program is loaded, not only a driver mode
one. Only release our reference on adapter free instead of
immediately after netdev unregister to allow offload to be disabled
first.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net.h | 2 ++
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 20 ++++++++++----------
2 files changed, 12 insertions(+), 10 deletions(-)
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:29
Respect the XDP_FLAGS_HW_MODE. When it's set install the program
on the NIC and skip enabling XDP in the driver.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:31
Make use of just added XDP_ATTACHED_HW.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 2 ++
1 file changed, 2 insertions(+)
From: Jakub Kicinski <hidden> Date: 2017-06-16 23:58:35
Extend the XDP_ATTACHED_* values to include offloaded mode.
Let drivers report whether program is installed in the driver
or the HW by changing the prog_attached field from bool to
u8 (type of the netlink attribute).
Exploit the fact that the value of XDP_ATTACHED_DRV is 1,
therefore since all drivers currently assign the mode with
double negation:
mode = !!xdp_prog;
no drivers have to be modified.
Signed-off-by: Jakub Kicinski <redacted>
---
include/linux/netdevice.h | 7 ++++---
include/uapi/linux/if_link.h | 1 +
net/core/dev.c | 3 +--
net/core/rtnetlink.c | 6 +++---
4 files changed, 9 insertions(+), 8 deletions(-)
@@ -809,7 +809,8 @@ enum xdp_netdev_command {XDP_SETUP_PROG,XDP_SETUP_PROG_HW,/* Check if a bpf program is set on the device. The callee should-*returntrueifaprogramiscurrentlyattachedandrunning.+*set@prog_attachedtooneofXDP_ATTACHED_*values,notethat"true"+*isequivalenttoXDP_ATTACHED_DRV.*/XDP_QUERY_PROG,};
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 22:55:46
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
Add an installation-time flag for requesting that the program
be installed only if it can be offloaded to HW.
Internally new command for ndo_xdp is added, this way we avoid
putting checks into drivers since they all return -EINVAL on
an unknown command.
Signed-off-by: Jakub Kicinski <redacted>
One thing I'm not sure I follow is that while you pass flags to the ndo
in patch 1, add a new XDP_SETUP_PROG_HW command here in patch 2 based on
the flags, and later on in patch 6, you don't really make use of it, but
look at the flags anyway? Then, why adding separate XDP_SETUP_PROG_HW
in the first place?
[patch 6:]
@@ -3338,6 +3341,7 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_xdp *xdp) switch (xdp->command) { case XDP_SETUP_PROG:+ case XDP_SETUP_PROG_HW: return nfp_net_xdp_setup(nn, xdp->prog, xdp->flags, xdp->extack);
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:23:11
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
The xdp_prog member of the adapter's data path structure is used
for XDP in driver mode. In case a XDP program is loaded with in
HW-only mode, we need to store it somewhere else. Add a new XDP
prog pointer in the main structure and use that when we need to
know whether any XDP program is loaded, not only a driver mode
one. Only release our reference on adapter free instead of
immediately after netdev unregister to allow offload to be disabled
first.
Signed-off-by: Jakub Kicinski <redacted>
Can you elaborate on the extra reference on the prog? So in
nfp_net_xdp_setup(), assuming a prog was already loaded on
driver side: after your set, nfp_net_xdp_setup_drv() will then
do the xchg() on nn->dp.xdp_prog, bpf_prog_put() this one and
later back in nfp_net_xdp_setup() we check nn->xdp_prog and
bpf_prog_put() it if it existed before and update nn->xdp_prog
to the current prog. So you end up with two puts on the same
program, but I don't see where you take the one additional ref
aside from the ref that you already get from dev_change_xdp_fd().
What am I missing?
From: Jakub Kicinski <hidden> Date: 2017-06-19 23:25:04
On Tue, 20 Jun 2017 00:55:41 +0200, Daniel Borkmann wrote:
quoted hunk
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
quoted
Add an installation-time flag for requesting that the program
be installed only if it can be offloaded to HW.
Internally new command for ndo_xdp is added, this way we avoid
putting checks into drivers since they all return -EINVAL on
an unknown command.
Signed-off-by: Jakub Kicinski <redacted>
One thing I'm not sure I follow is that while you pass flags to the ndo
in patch 1, add a new XDP_SETUP_PROG_HW command here in patch 2 based on
the flags, and later on in patch 6, you don't really make use of it, but
look at the flags anyway? Then, why adding separate XDP_SETUP_PROG_HW
in the first place?
[patch 6:]
@@ -3338,6 +3341,7 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_xdp *xdp) switch (xdp->command) { case XDP_SETUP_PROG:+ case XDP_SETUP_PROG_HW: return nfp_net_xdp_setup(nn, xdp->prog, xdp->flags, xdp->extack);
We still need the flags to be able to differentiate between default/no
flags case where we load to the driver and the HW ("both"), and when
the DRV_MODE flag is set, in which case we disable the HW offload and
only load to the driver. We have three cases:
drv offload
no flag yes attempted
DRV_MODE yes no
HW_MODE no yes
The XDP_SETUP_PROG_HW command is purely for convenience of drivers
without an offload. I felt it's not appropriate to burden all drivers
with:
if (xdp->flags & XDP_FLAGS_HW_MODE)
return -EOPNOTSUPP;
But, I do have a patch which does it, so I'm happy to drop the new
command if it's preferred.
From: Jakub Kicinski <hidden> Date: 2017-06-19 23:34:44
On Tue, 20 Jun 2017 01:23:05 +0200, Daniel Borkmann wrote:
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
quoted
The xdp_prog member of the adapter's data path structure is used
for XDP in driver mode. In case a XDP program is loaded with in
HW-only mode, we need to store it somewhere else. Add a new XDP
prog pointer in the main structure and use that when we need to
know whether any XDP program is loaded, not only a driver mode
one. Only release our reference on adapter free instead of
immediately after netdev unregister to allow offload to be disabled
first.
Signed-off-by: Jakub Kicinski <redacted>
Can you elaborate on the extra reference on the prog?
Sorry, this patch went through a few revisions and the subject doesn't
express the intent too well any more :S Originally it was about making
sure we have a reference on the program when it's offloaded but not
loaded in the driver, but I realized the we have the reference from
dev_change_xdp_fd() already, so now the patch just releases the
reference on the offloaded program.
So in nfp_net_xdp_setup(), assuming a prog was already loaded on
driver side: after your set, nfp_net_xdp_setup_drv() will then
do the xchg() on nn->dp.xdp_prog, bpf_prog_put() this one and
later back in nfp_net_xdp_setup() we check nn->xdp_prog and
bpf_prog_put() it if it existed before and update nn->xdp_prog
to the current prog. So you end up with two puts on the same
program, but I don't see where you take the one additional ref
aside from the ref that you already get from dev_change_xdp_fd().
What am I missing?
You are right, I missed there were two spots where I was doing a
bpf_prog_put() in nfp_net_xdp_setup_drv(), thanks!
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:36:29
On 06/20/2017 01:24 AM, Jakub Kicinski wrote:
[...]
The XDP_SETUP_PROG_HW command is purely for convenience of drivers
without an offload. I felt it's not appropriate to burden all drivers
with:
if (xdp->flags & XDP_FLAGS_HW_MODE)
return -EOPNOTSUPP;
But, I do have a patch which does it, so I'm happy to drop the new
command if it's preferred.
Ahh, that makes sense, yep. I was only focused on reviewing this in
the context of nfp driver. Lack of coffee. ;)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:38:41
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
Pass XDP flags to the xdp ndo. This will allow drivers to look
at the mode flags and make decisions about offload.
Signed-off-by: Jakub Kicinski <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:39:22
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
Add an installation-time flag for requesting that the program
be installed only if it can be offloaded to HW.
Internally new command for ndo_xdp is added, this way we avoid
putting checks into drivers since they all return -EINVAL on
an unknown command.
Signed-off-by: Jakub Kicinski <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:40:19
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
Extend the XDP_ATTACHED_* values to include offloaded mode.
Let drivers report whether program is installed in the driver
or the HW by changing the prog_attached field from bool to
u8 (type of the netlink attribute).
Exploit the fact that the value of XDP_ATTACHED_DRV is 1,
therefore since all drivers currently assign the mode with
double negation:
mode = !!xdp_prog;
no drivers have to be modified.
Signed-off-by: Jakub Kicinski <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-19 23:50:23
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
quoted hunk
Respect the XDP_FLAGS_HW_MODE. When it's set install the program
on the NIC and skip enabling XDP in the driver.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Can you make this assumption here? If dev_change_xdp_fd() is called
without XDP_FLAGS_HW_MODE or XDP_FLAGS_DRV_MODE flags, then we set prog
to both, drv_prog and offload_prog. Is this expected?
Maybe in nfp_net_xdp_setup() check for !hweight32(xdp_flags & XDP_FLAGS_MODES)
and then set flags |= XDP_FLAGS_DRV_MODE before both assignments?
From: Jakub Kicinski <hidden> Date: 2017-06-20 00:01:31
On Tue, 20 Jun 2017 01:50:17 +0200, Daniel Borkmann wrote:
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
quoted
Respect the XDP_FLAGS_HW_MODE. When it's set install the program
on the NIC and skip enabling XDP in the driver.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Can you make this assumption here? If dev_change_xdp_fd() is called
without XDP_FLAGS_HW_MODE or XDP_FLAGS_DRV_MODE flags, then we set prog
to both, drv_prog and offload_prog. Is this expected?
Maybe in nfp_net_xdp_setup() check for !hweight32(xdp_flags & XDP_FLAGS_MODES)
and then set flags |= XDP_FLAGS_DRV_MODE before both assignments?
I thought we did want both. In case the program is loaded to both the
HW/FW will mark the packets with BPF bit in the descriptor so that they
are not processed twice. But the driver path will be configured for
running bpf and when user replaces the program with one which cannot be
offloaded the driver will not have to reconfigure itself.
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-06-20 00:09:59
On 06/20/2017 02:01 AM, Jakub Kicinski wrote:
On Tue, 20 Jun 2017 01:50:17 +0200, Daniel Borkmann wrote:
quoted
On 06/17/2017 01:57 AM, Jakub Kicinski wrote:
quoted
Respect the XDP_FLAGS_HW_MODE. When it's set install the program
on the NIC and skip enabling XDP in the driver.
Signed-off-by: Jakub Kicinski <redacted>
---
drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Can you make this assumption here? If dev_change_xdp_fd() is called
without XDP_FLAGS_HW_MODE or XDP_FLAGS_DRV_MODE flags, then we set prog
to both, drv_prog and offload_prog. Is this expected?
Maybe in nfp_net_xdp_setup() check for !hweight32(xdp_flags & XDP_FLAGS_MODES)
and then set flags |= XDP_FLAGS_DRV_MODE before both assignments?
I thought we did want both. In case the program is loaded to both the
HW/FW will mark the packets with BPF bit in the descriptor so that they
are not processed twice. But the driver path will be configured for
running bpf and when user replaces the program with one which cannot be
offloaded the driver will not have to reconfigure itself.
Okay, that's a good point ... so that you can just use xchg() later on.
Probably worth explaining this rationale in a short comment.