Re: [PATCH net-next v4 10/10] tsnep: Support XDP BPF program setup

On 10.01.23 18:33, Alexander H Duyck wrote:

On Mon, 2023-01-09 at 20:15 +0100, Gerhard Engleder wrote:

quoted

Implement setup of BPF programs for XDP RX path with command
XDP_SETUP_PROG of ndo_bpf(). This is the final step 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.

Additionally remove $(tsnep-y) from $(tsnep-objs) because it is added
automatically.

Test results with A53 1.2GHz:

XDP_DROP (samples/bpf/xdp1)
proto 17:     883878 pkt/s

XDP_TX (samples/bpf/xdp2)
proto 17:     255693 pkt/s

XDP_REDIRECT (samples/bpf/xdpsock)
  sock0@eth2:0 rxdrop xdp-drv
                    pps            pkts           1.00
rx                 855,582        5,404,523
tx                 0              0

XDP_REDIRECT (samples/bpf/xdp_redirect)
eth2->eth1         613,267 rx/s   0 err,drop/s   613,272 xmit/s

Signed-off-by: Gerhard Engleder <redacted>
---
  drivers/net/ethernet/engleder/Makefile     |  2 +-
  drivers/net/ethernet/engleder/tsnep.h      |  6 +++++
  drivers/net/ethernet/engleder/tsnep_main.c | 25 ++++++++++++++++---
  drivers/net/ethernet/engleder/tsnep_xdp.c  | 29 ++++++++++++++++++++++
  4 files changed, 58 insertions(+), 4 deletions(-)
  create mode 100644 drivers/net/ethernet/engleder/tsnep_xdp.c

<...>

quoted

--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c

@@ -1373,7 +1373,7 @@ static void tsnep_free_irq(struct tsnep_queue *queue, bool first)
  	memset(queue->name, 0, sizeof(queue->name));
  }
  
-static int tsnep_netdev_open(struct net_device *netdev)
+int tsnep_netdev_open(struct net_device *netdev)
  {
  	struct tsnep_adapter *adapter = netdev_priv(netdev);
  	int tx_queue_index = 0;

@@ -1436,6 +1436,8 @@ static int tsnep_netdev_open(struct net_device *netdev)
  		tsnep_enable_irq(adapter, adapter->queue[i].irq_mask);
  	}
  
+	netif_tx_start_all_queues(adapter->netdev);
+
  	clear_bit(__TSNEP_DOWN, &adapter->state);
  
  	return 0;

@@ -1457,12 +1459,16 @@ static int tsnep_netdev_open(struct net_device *netdev)
  	return retval;
  }
  
-static int tsnep_netdev_close(struct net_device *netdev)
+int tsnep_netdev_close(struct net_device *netdev)
  {
  	struct tsnep_adapter *adapter = netdev_priv(netdev);
  	int i;
  
-	set_bit(__TSNEP_DOWN, &adapter->state);
+	if (test_and_set_bit(__TSNEP_DOWN, &adapter->state))
+		return 0;
+
+	netif_carrier_off(netdev);
+	netif_tx_stop_all_queues(netdev);
  

As I called out earlier the __TSNEP_DOWN is just !IFF_UP so you don't
need that bit.

The fact that netif_carrier_off is here also points out the fact that
the code in the Tx path isn't needed regarding __TSNEP_DOWN and you can
probably just check netif_carrier_ok if you need the check.

tsnep_netdev_close() is called directly during bpf prog setup (see
tsnep_xdp_setup_prog() in this commit). If the following
tsnep_netdev_open() call fails, then this flag signals that the device
is already down and nothing needs to be cleaned up if
tsnep_netdev_close() is called later (because IFF_UP is still set).

Thanks for the review!

Gerhard