mbox series

[0/8] New netdev feature flags for XDP

Message ID 20201116093452.7541-1-marekx.majtyka@intel.com (mailing list archive)
Headers show
Series New netdev feature flags for XDP | expand

Message

Marek Majtyka Nov. 16, 2020, 9:34 a.m. UTC
From: Marek Majtyka <marekx.majtyka@intel.com>

Implement support for checking if a netdev has native XDP and AF_XDP zero
copy support. Previously, there was no way to do this other than to try
to create an AF_XDP socket on the interface or load an XDP program and
see if it worked. This commit changes this by extending existing
netdev_features in the following way:
 * xdp        - full XDP support (XDP_{TX, PASS, DROP, ABORT, REDIRECT})
 * af-xdp-zc  - AF_XDP zero copy support
NICs supporting these features are updated by turning the corresponding
netdev feature flags on.

NOTE:
 Only the compilation check was performed for:
  - ice, 
  - igb,
  - mlx5, 
  - bnxt, 
  - dpaa2, 
  - mvmeta, 
  - mvpp2, 
  - qede,
  - sfc, 
  - netsec, 
  - cpsw, 
  - xen, 
  - virtio_net.

Libbpf library is extended in order to provide a simple API for gathering
information about XDP supported capabilities of a netdev. This API
utilizes netlink interface towards kernel. With this function it is
possible to get xsk supported options for netdev beforehand.
The new API is used in core xsk code as well as in the xdpsock sample.

These new flags also solve the problem with strict recognition of zero
copy support. The problem is that there are drivers out there that only
support XDP partially, so it is possible to successfully load the XDP
program in native mode, but it will still not be able to support zero-copy
as it does not have XDP_REDIRECT support. With af-xdp-zc flag the check
is possible and trivial.

Marek Majtyka (8):
  net: ethtool: extend netdev_features flag set
  drivers/net: turn XDP flags on
  xsk: add usage of xdp netdev_features flags
  xsk: add check for full support of XDP in bind
  libbpf: extend netlink attribute API
  libbpf: add functions to get XSK modes
  libbpf: add API to get XSK/XDP caps
  samples/bpf/xdp: apply netdev XDP/XSK modes info

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   1 +
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |   1 +
 drivers/net/ethernet/intel/i40e/i40e_main.c   |   2 +
 drivers/net/ethernet/intel/ice/ice_main.c     |   4 +
 drivers/net/ethernet/intel/igb/igb_main.c     |   2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   3 +
 drivers/net/ethernet/marvell/mvneta.c         |   1 +
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   1 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   2 +
 drivers/net/ethernet/qlogic/qede/qede_main.c  |   1 +
 drivers/net/ethernet/sfc/efx.c                |   1 +
 drivers/net/ethernet/socionext/netsec.c       |   1 +
 drivers/net/ethernet/ti/cpsw.c                |   2 +
 drivers/net/ethernet/ti/cpsw_new.c            |   2 +
 drivers/net/tun.c                             |   4 +
 drivers/net/veth.c                            |   1 +
 drivers/net/virtio_net.c                      |   1 +
 drivers/net/xen-netfront.c                    |   1 +
 include/linux/netdev_features.h               |   6 +
 include/net/xdp.h                             |  13 +
 include/net/xdp_sock_drv.h                    |  11 +
 include/uapi/linux/if_xdp.h                   |   1 +
 net/ethtool/common.c                          |   2 +
 net/xdp/xsk.c                                 |   4 +-
 net/xdp/xsk_buff_pool.c                       |  21 +-
 samples/bpf/xdpsock_user.c                    | 117 +++++-
 tools/include/uapi/linux/ethtool.h            |  44 ++
 tools/include/uapi/linux/if_xdp.h             |   1 +
 tools/lib/bpf/ethtool.h                       |  49 +++
 tools/lib/bpf/libbpf.h                        |   1 +
 tools/lib/bpf/libbpf.map                      |   1 +
 tools/lib/bpf/netlink.c                       | 379 +++++++++++++++++-
 tools/lib/bpf/nlattr.c                        | 105 +++++
 tools/lib/bpf/nlattr.h                        |  22 +
 tools/lib/bpf/xsk.c                           |  54 ++-
 tools/lib/bpf/xsk.h                           |   3 +-
 36 files changed, 845 insertions(+), 20 deletions(-)
 create mode 100644 tools/lib/bpf/ethtool.h

Comments

Toke Høiland-Jørgensen Nov. 16, 2020, 1:25 p.m. UTC | #1
alardam@gmail.com writes:

> From: Marek Majtyka <marekx.majtyka@intel.com>
>
> Implement support for checking if a netdev has native XDP and AF_XDP zero
> copy support. Previously, there was no way to do this other than to try
> to create an AF_XDP socket on the interface or load an XDP program and
> see if it worked. This commit changes this by extending existing
> netdev_features in the following way:
>  * xdp        - full XDP support (XDP_{TX, PASS, DROP, ABORT, REDIRECT})
>  * af-xdp-zc  - AF_XDP zero copy support
> NICs supporting these features are updated by turning the corresponding
> netdev feature flags on.

Thank you for working on this! The lack of a way to discover whether an
interface supports XDP is really annoying.

However, I don't think just having two separate netdev feature flags for
XDP and AF_XDP is going to cut it. Whatever mechanism we end up will
need to be able to express at least the following, in addition to your
two flags:

- Which return codes does it support (with DROP/PASS, TX and REDIRECT as
  separate options)?
- Does this interface be used as a target for XDP_REDIRECT
  (supported/supported but not enabled)?
- Does the interface support offloaded XDP?

That's already five or six more flags, and we can't rule out that we'll
need more; so I'm not sure if just defining feature bits for all of them
is a good idea.

In addition, we should be able to check this in a way so we can reject
XDP programs that use features that are not supported. E.g., program
uses REDIRECT return code (or helper), but the interface doesn't support
it? Reject at attach/load time! Or the user attempts to insert an
interface into a redirect map, but that interface doesn't implement
ndo_xdp_xmit()? Reject the insert! Etc.

That last bit can be added later, of course, but we need to make sure we
design the support in a way that it is possible to do so...

-Toke
Magnus Karlsson Nov. 17, 2020, 7:37 a.m. UTC | #2
On Mon, Nov 16, 2020 at 2:25 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> alardam@gmail.com writes:
>
> > From: Marek Majtyka <marekx.majtyka@intel.com>
> >
> > Implement support for checking if a netdev has native XDP and AF_XDP zero
> > copy support. Previously, there was no way to do this other than to try
> > to create an AF_XDP socket on the interface or load an XDP program and
> > see if it worked. This commit changes this by extending existing
> > netdev_features in the following way:
> >  * xdp        - full XDP support (XDP_{TX, PASS, DROP, ABORT, REDIRECT})
> >  * af-xdp-zc  - AF_XDP zero copy support
> > NICs supporting these features are updated by turning the corresponding
> > netdev feature flags on.
>
> Thank you for working on this! The lack of a way to discover whether an
> interface supports XDP is really annoying.
>
> However, I don't think just having two separate netdev feature flags for
> XDP and AF_XDP is going to cut it. Whatever mechanism we end up will
> need to be able to express at least the following, in addition to your
> two flags:
>
> - Which return codes does it support (with DROP/PASS, TX and REDIRECT as
>   separate options)?
> - Does this interface be used as a target for XDP_REDIRECT
>   (supported/supported but not enabled)?
> - Does the interface support offloaded XDP?

If we want feature discovery on this level, which seems to be a good
idea and goal to have, then it is a dead end to bunch all XDP features
into one. But fortunately, this can easily be addressed.

> That's already five or six more flags, and we can't rule out that we'll
> need more; so I'm not sure if just defining feature bits for all of them
> is a good idea.

I think this is an important question. Is extending the netdev
features flags the right way to go? If not, is there some other
interface in the kernel that could be used/extended for this? If none
of these are possible, then we (unfortunately) need a new interface
and in that case, what should it look like?

Thanks for taking a look at this Toke.

> In addition, we should be able to check this in a way so we can reject
> XDP programs that use features that are not supported. E.g., program
> uses REDIRECT return code (or helper), but the interface doesn't support
> it? Reject at attach/load time! Or the user attempts to insert an
> interface into a redirect map, but that interface doesn't implement
> ndo_xdp_xmit()? Reject the insert! Etc.
>
> That last bit can be added later, of course, but we need to make sure we
> design the support in a way that it is possible to do so...
>
> -Toke
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Marek Majtyka Nov. 17, 2020, 8:55 a.m. UTC | #3
On Tue, Nov 17, 2020 at 8:37 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:

Thank you for your quick answers and comments.

>
> On Mon, Nov 16, 2020 at 2:25 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > alardam@gmail.com writes:
> >
> > > From: Marek Majtyka <marekx.majtyka@intel.com>
> > >
> > > Implement support for checking if a netdev has native XDP and AF_XDP zero
> > > copy support. Previously, there was no way to do this other than to try
> > > to create an AF_XDP socket on the interface or load an XDP program and
> > > see if it worked. This commit changes this by extending existing
> > > netdev_features in the following way:
> > >  * xdp        - full XDP support (XDP_{TX, PASS, DROP, ABORT, REDIRECT})
> > >  * af-xdp-zc  - AF_XDP zero copy support
> > > NICs supporting these features are updated by turning the corresponding
> > > netdev feature flags on.
> >
> > Thank you for working on this! The lack of a way to discover whether an
> > interface supports XDP is really annoying.
> >
> > However, I don't think just having two separate netdev feature flags for
> > XDP and AF_XDP is going to cut it. Whatever mechanism we end up will
> > need to be able to express at least the following, in addition to your
> > two flags:
> >
> > - Which return codes does it support (with DROP/PASS, TX and REDIRECT as
> >   separate options)?
> > - Does this interface be used as a target for XDP_REDIRECT
> >   (supported/supported but not enabled)?
> > - Does the interface support offloaded XDP?
>
> If we want feature discovery on this level, which seems to be a good
> idea and goal to have, then it is a dead end to bunch all XDP features
> into one. But fortunately, this can easily be addressed.

Do you think that is it still considerable to have a single netdev
flag that means "some" XDP feature support which would activate new
further functionalities?

>
> > That's already five or six more flags, and we can't rule out that we'll
> > need more; so I'm not sure if just defining feature bits for all of them
> > is a good idea.
>
> I think this is an important question. Is extending the netdev
> features flags the right way to go? If not, is there some other
> interface in the kernel that could be used/extended for this? If none
> of these are possible, then we (unfortunately) need a new interface
> and in that case, what should it look like?

Toke, are you thinking about any particular existing interface or a
new specific one?

>
> Thanks for taking a look at this Toke.
>
> > In addition, we should be able to check this in a way so we can reject
> > XDP programs that use features that are not supported. E.g., program
> > uses REDIRECT return code (or helper), but the interface doesn't support
> > it? Reject at attach/load time! Or the user attempts to insert an
> > interface into a redirect map, but that interface doesn't implement
> > ndo_xdp_xmit()? Reject the insert! Etc.
> >
> > That last bit can be added later, of course, but we need to make sure we
> > design the support in a way that it is possible to do so...
> >
> > -Toke
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@osuosl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Toke Høiland-Jørgensen Nov. 17, 2020, 6:38 p.m. UTC | #4
Marek Majtyka <alardam@gmail.com> writes:

> On Tue, Nov 17, 2020 at 8:37 AM Magnus Karlsson
> <magnus.karlsson@gmail.com> wrote:
>
> Thank you for your quick answers and comments.
>
>>
>> On Mon, Nov 16, 2020 at 2:25 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > alardam@gmail.com writes:
>> >
>> > > From: Marek Majtyka <marekx.majtyka@intel.com>
>> > >
>> > > Implement support for checking if a netdev has native XDP and AF_XDP zero
>> > > copy support. Previously, there was no way to do this other than to try
>> > > to create an AF_XDP socket on the interface or load an XDP program and
>> > > see if it worked. This commit changes this by extending existing
>> > > netdev_features in the following way:
>> > >  * xdp        - full XDP support (XDP_{TX, PASS, DROP, ABORT, REDIRECT})
>> > >  * af-xdp-zc  - AF_XDP zero copy support
>> > > NICs supporting these features are updated by turning the corresponding
>> > > netdev feature flags on.
>> >
>> > Thank you for working on this! The lack of a way to discover whether an
>> > interface supports XDP is really annoying.
>> >
>> > However, I don't think just having two separate netdev feature flags for
>> > XDP and AF_XDP is going to cut it. Whatever mechanism we end up will
>> > need to be able to express at least the following, in addition to your
>> > two flags:
>> >
>> > - Which return codes does it support (with DROP/PASS, TX and REDIRECT as
>> >   separate options)?
>> > - Does this interface be used as a target for XDP_REDIRECT
>> >   (supported/supported but not enabled)?
>> > - Does the interface support offloaded XDP?
>>
>> If we want feature discovery on this level, which seems to be a good
>> idea and goal to have, then it is a dead end to bunch all XDP features
>> into one. But fortunately, this can easily be addressed.
>
> Do you think that is it still considerable to have a single netdev
> flag that means "some" XDP feature support which would activate new
> further functionalities?

Why bother? The presence of any XDP-specific feature bits would imply
the support for XDP :)

>> > That's already five or six more flags, and we can't rule out that we'll
>> > need more; so I'm not sure if just defining feature bits for all of them
>> > is a good idea.
>>
>> I think this is an important question. Is extending the netdev
>> features flags the right way to go? If not, is there some other
>> interface in the kernel that could be used/extended for this? If none
>> of these are possible, then we (unfortunately) need a new interface
>> and in that case, what should it look like?
>
> Toke, are you thinking about any particular existing interface or a
> new specific one?

I have mostly been thinking about the internal kernel interface. The
simple thing would just be to define a whole new bitmap of XDP-specific
feature bits that the rest of the kernel can consume. That would also
mean we don't have to do pointer chasing to see if the ndos are
implemented, which Jesper pointed out the other day actually shows up on
his profiling traces.

The downside to having them be feature flags is that they can get out of
sync, of course. But if we block the support from working unless the
right flags are set, that should at least make driver developers pay
attention. Although we'd have to change all the drivers in one go, but I
suppose that's not too onerous seeing as you just did that for this
series :)

So what that boils down to is basically what you're doing in this
series, but more fine grained, via a new netdev->xdp_features instead of
burning bits in netdev->features.

As for UAPI, i dunno? Ethtool is netlink now, right? So it should be
fairly easy to just extend with a new attribute for XDP?

I believe there was originally some resistance to explicitly exposing
XDP capabilities to userspace because we wanted all drivers to implement
all features. Clearly that has not panned out, though, so as far as I'm
concerned we can just expose it and be done with it :) But I'll let
others weigh in here; the original discussions predate my involvement.

-Toke