"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote on 06/02/2011 08:13:46 PM:
> > Please review this patch to see if it looks reasonable:
>
> Hmm, since you decided to work on top of my patch,
> I'd appreciate split-up fixes.
OK (that also explains your next comment).
> > 1. Picked comments/code from MST's code and Rusty's review.
> > 2. virtqueue_min_capacity() needs to be called only if it returned
> > empty the last time it was called.
> > 3. Fix return value bug in free_old_xmit_skbs (hangs guest).
> > 4. Stop queue only if capacity is not enough for next xmit.
>
> That's what we always did ...
I had made the patch against your patch, hence this change (sorry for
the confusion!).
> > 5. Fix/clean some likely/unlikely checks (hopefully).
> >
> > I have done some minimal netperf tests with this.
> >
> > With this patch, add_buf returning capacity seems to be useful - it
> > allows less virtio API calls.
>
> Why bother? It's cheap ...
If add_buf retains it's functionality to return the capacity (it
is going to need a change to return 0 otherwise anyway), is it
useful to call another function at each xmit?
> > +static bool free_old_xmit_skbs(struct virtnet_info *vi, int to_free)
> > +{
> > + bool empty = virtqueue_min_capacity(vi->svq) < MAX_SKB_FRAGS + 2;
> > +
> > + do {
> > + if (!free_one_old_xmit_skb(vi)) {
> > + /* No more skbs to free up */
> > break;
> > - pr_debug("Sent skb %p\n", skb);
> > - vi->dev->stats.tx_bytes += skb->len;
> > - vi->dev->stats.tx_packets++;
> > - dev_kfree_skb_any(skb);
> > - }
> > - return r;
> > + }
> > +
> > + if (empty) {
> > + /* Check again if there is enough space */
> > + empty = virtqueue_min_capacity(vi->svq) <
> > + MAX_SKB_FRAGS + 2;
> > + } else {
> > + --to_free;
> > + }
> > + } while (to_free > 0);
> > +
> > + return !empty;
> > }
>
> Why bother doing the capacity check in this function?
To return whether we have enough space for next xmit. It should call
it only once unless space is running out. Does it sound OK?
> > - if (unlikely(ret < 0)) {
> > + if (unlikely(capacity < 0)) {
> > + /*
> > + * Failure to queue should be impossible. The only way to
> > + * reach here is if we got a cb before 3/4th of space was
> > + * available. We could stop the queue and re-enable
> > + * callbacks (and possibly return TX_BUSY), but we don't
> > + * bother since this is impossible.
>
> It's far from impossible. The 3/4 thing is only a hint, and old devices
> don't support it anyway.
OK, I will re-put back your comment.
> > - if (!likely(free_old_xmit_skbs(vi, 2))) {
> > - netif_stop_queue(dev);
> > - if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
> > - /* More just got used, free them and recheck. */
> > - if (!likely(free_old_xmit_skbs(vi, 0))) {
> > - netif_start_queue(dev);
> > - virtqueue_disable_cb(vi->svq);
> > + /*
> > + * Apparently nice girls don't return TX_BUSY; check capacity and
> > + * stop the queue before it gets out of hand. Naturally, this
wastes
> > + * entries.
> > + */
> > + if (capacity < 2+MAX_SKB_FRAGS) {
> > + /*
> > + * We don't have enough space for the next packet. Try
> > + * freeing more.
> > + */
> > + if (likely(!free_old_xmit_skbs(vi, UINT_MAX))) {
> > + netif_stop_queue(dev);
> > + if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
> > + /* More just got used, free them and recheck. */
> > + if (likely(free_old_xmit_skbs(vi, UINT_MAX))) {
>
> Is this where the bug was?
Return value in free_old_xmit() was wrong. I will re-do against the
mainline kernel.
Thanks,
- KK
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
