[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2eaa20bb-2fde-fe6f-5dde-f84bad49a987@intel.com>
Date: Thu, 5 Jan 2023 18:24:14 +0100
From: Alexander Lobakin <alexandr.lobakin@...el.com>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
<pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 6/9] tsnep: Support XDP BPF program setup
From: Gerhard Engleder <gerhard@...leder-embedded.com>
Date: Wed Jan 04 2023 20:41:29 GMT+0100
> Implement setup of BPF programs for XDP RX path with command
> XDP_SETUP_PROG of ndo_bpf(). This is prework for XDP RX path support.
>
> tsnep_netdev_close() is called directly during BPF program setup. Add
> netif_carrier_off() and netif_tx_stop_all_queues() calls to signal to
> network stack that device is down. Otherwise network stack would
> continue transmitting pakets.
>
> Return value of tsnep_netdev_open() is not checked during BPF program
> setup like in other drivers. Forwarding the return value would result in
> a bpf_prog_put() call in dev_xdp_install(), which would make removal of
> BPF program necessary.
>
> If tsnep_netdev_open() fails during BPF program setup, then the network
> stack would call tsnep_netdev_close() anyway. Thus, tsnep_netdev_close()
> checks now if device is already down.
>
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
> drivers/net/ethernet/engleder/Makefile | 2 +-
> drivers/net/ethernet/engleder/tsnep.h | 13 +++++++++++
> drivers/net/ethernet/engleder/tsnep_main.c | 25 +++++++++++++++++---
> drivers/net/ethernet/engleder/tsnep_xdp.c | 27 ++++++++++++++++++++++
> 4 files changed, 63 insertions(+), 4 deletions(-)
> create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c
>
> diff --git a/drivers/net/ethernet/engleder/Makefile b/drivers/net/ethernet/engleder/Makefile
> index b6e3b16623de..0901801cfcc9 100644
> --- a/drivers/net/ethernet/engleder/Makefile
> +++ b/drivers/net/ethernet/engleder/Makefile
> @@ -6,5 +6,5 @@
> obj-$(CONFIG_TSNEP) += tsnep.o
>
> tsnep-objs := tsnep_main.o tsnep_ethtool.o tsnep_ptp.o tsnep_tc.o \
> - tsnep_rxnfc.o $(tsnep-y)
> + tsnep_rxnfc.o tsnep_xdp.o $(tsnep-y)
Not related directly to the subject, but could be fixed in that commit I
hope: you don't need to add $(tsnep-y) to $(tsnep-objs), it gets added
automatically.
> tsnep-$(CONFIG_TSNEP_SELFTESTS) += tsnep_selftests.o
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index 29b04127f529..0e7fc36a64e1 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h
[...]
> @@ -215,6 +220,14 @@ int tsnep_rxnfc_add_rule(struct tsnep_adapter *adapter,
> int tsnep_rxnfc_del_rule(struct tsnep_adapter *adapter,
> struct ethtool_rxnfc *cmd);
>
> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> + struct netlink_ext_ack *extack);
> +
> +static inline bool tsnep_xdp_is_enabled(struct tsnep_adapter *adapter)
> +{
> + return !!adapter->xdp_prog;
Any concurrent access protection? READ_ONCE(), RCU?
> +}
> +
> #if IS_ENABLED(CONFIG_TSNEP_SELFTESTS)
> int tsnep_ethtool_get_test_count(void);
> void tsnep_ethtool_get_test_strings(u8 *data);
[...]
> +static int tsnep_netdev_bpf(struct net_device *dev, struct netdev_bpf *bpf)
> +{
> + struct tsnep_adapter *adapter = netdev_priv(dev);
> +
> + switch (bpf->command) {
> + case XDP_SETUP_PROG:
> + return tsnep_xdp_setup_prog(adapter, bpf->prog, bpf->extack);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
So, after this commit, I'm able to install an XDP prog to an interface,
but it won't do anything. I think the patch could be moved to the end of
the series, so that it won't end up with such?
> +
> static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
> struct xdp_frame **xdp, u32 flags)
> {
[...]
> +int tsnep_xdp_setup_prog(struct tsnep_adapter *adapter, struct bpf_prog *prog,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_device *dev = adapter->netdev;
> + bool if_running = netif_running(dev);
> + struct bpf_prog *old_prog;
> +
> + if (if_running)
> + tsnep_netdev_close(dev);
You don't need to close the interface if `!prog == !old_prog`. I
wouldn't introduce redundant down-ups here and leave no possibility for
prog hotswapping.
> +
> + old_prog = xchg(&adapter->xdp_prog, prog);
> + if (old_prog)
> + bpf_prog_put(old_prog);
> +
> + if (if_running)
> + tsnep_netdev_open(dev);
> +
> + return 0;
> +}
Thanks,
Olek
Powered by blists - more mailing lists