On Thu, 2023-06-01 at 09:10 -0700, Jakub Kicinski wrote: > On Thu, 01 Jun 2023 10:41:34 +0200 Paolo Abeni wrote: > > > I agree that this is the correct return value for this case. > > > But I do wonder if, as per the documentation of ndo_start_xmit, > > > something should be done to avoid getting into such a situation. > > > > > > * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, > > > * struct net_device *dev); > > > * Called when a packet needs to be transmitted. > > > * Returns NETDEV_TX_OK. Can return NETDEV_TX_BUSY, but you should stop > > > * the queue before that can happen; it's for obsolete devices and weird > > > * corner cases, but the stack really does a non-trivial amount > > > * of useless work if you return NETDEV_TX_BUSY. > > > * Required; cannot be NULL. > > > > I agree with Simon, it looks like the driver usage of > > netif_stop_subqueue()/netif_wake_subqueue() is a dubious. > > > > I think you will be better of using > > netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() alike what > > rtl8169 is doing. e.g. netif_subqueue_maybe_stop() should be invoked > > after the tx buffer enqueue, and netif_subqueue_completed_wake() should > > be invoked after successful tx ring cleanup. > > That's a separate issue, tho, right? The cleanup is lockless and our > magic lockless macro scheme does not protect from spurious wakeups. > So they still need to check if the queue is full at the top of xmit. > And they still need to return the correct error in that case.. I guess you are right, dubious wakeup could be addresses with a separate patch if needed. Fine by me to apply it as-is. Cheers, Paolo
