On Friday 08 January 2010 00:06, Marc Kleine-Budde wrote:
> Oliver Hartkopp wrote:
> > Matthias Fuchs wrote:
> > 
> >> But I ran into some issues. Perhaps some of you can help:
> >> Q1: Under high TX load the driver runs out of free tx_contexts.
> >> (see "couldn't find free context"). I expected the socket can core
> >> to take care of this so that the lower driver cannot be overrun
> >> by tx jobs. Either this expectation is wrong or there's a bug in
> >> the driver :-)
> > 
> > Hi Matthias,
> > 
> > i wonder, why you do not stop the netdev tx queue immediately at the 
> > beginning of
> > 
> >> +static netdev_tx_t esd_usb2_start_xmit(struct sk_buff *skb,
> >> +                                struct net_device *netdev)
> > 
> > but at this (late) point:
> > 
> >> +  if (err) {
> >> +          can_free_echo_skb(netdev, context->echo_index);
> >> +
> >> +          atomic_dec(&priv->active_tx_urbs);
> >> +          usb_unanchor_urb(urb);
> >> +
> >> +          if (err == -ENODEV)
> >> +                  netif_device_detach(netdev);
> >> +          else
> >> +                  dev_warn(ND2D(netdev), "failed tx_urb %d\n", err);
> >> +
> >> +          goto releasebuf;
> >> +  } else {
> >> +          netdev->trans_start = jiffies;
> >> +
> >> +          /* Slow down tx path */
> >> +          if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
> >> +                  netif_stop_queue(netdev);
> >> +  }
> > 
> > The netdev queues in Linux will give you skbs until you say 'stop' :-)
> > This is not SocketCAN specific.
> > 
> > I assume the handling of netif_[wake|stop]_queue() should be reworked.
> > 
> > E.g. invoke netif_stop_queue() at the beginning of esd_usb2_start_xmit() and
> > when everything is really fine, you can invoke netif_wake_queue() at the and
> > of esd_usb2_start_xmit() again.
> 
> you don't have to make it that complicated. Just stop the queue at the
> end of your tx routine if you're out of tx contexts. And reenable the
> queue if you've finished your tx jobs.
This works just fine. And it brings me to the point that active_tx_urbs is
propably not the best name for that variable. It has not much
to do with URBs in this context. I will rename it into active_tx_jobs.

Matthias
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core

Reply via email to