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

On 11.01.23 01:12, Jakub Kicinski wrote:

On Tue, 10 Jan 2023 22:38:04 +0100 Gerhard Engleder wrote:

quoted

quoted

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).

TBH we've been pushing pretty hard for a while now to stop people
from implementing the:

	close()
	change config
	open()

sort of reconfiguration. I did that myself when I was a was
implementing my first Ethernet driver and DaveM nacked the change.
Must have been a decade ago.

Imagine you're working on a remote box via SSH and the box is under
transient memory pressure. Allocations fail, we don't want the machine
to fall off the network :(

I agree with you that this pattern is bad. Most XDP BPF program setup do
it like that, but this is of course no valid argument.

In the last review round I made the following suggestion (but got no
reply so far):

What about always using 'XDP_PACKET_HEADROOM' as offset in the RX
buffer? The offset 'NET_SKB_PAD + NET_IP_ALIGN' would not even be used
if XDP is not enabled. Changing this offset is the only task to be done
at the first XDP BFP prog setup call. By always using this offset
no

	close()
	change config
	open()

pattern is needed. As a result no handling for failed open() is needed
and __TSNEP_DOWN is not needed. Simpler code with less problems in my
opinion.

The only problem could be that NET_IP_ALIGN is not used, but
NET_IP_ALIGN is 0 anyway on the two platforms (x86, arm64) where this
driver is used.

Gerhard