On Sun, 2009-12-13 at 13:08 +0200, Michael S. Tsirkin wrote:
> Do not cast away void*.
> This initialization above looks very strange: in
> fact only one of skb, page makes sense.
> So I think you should either get rid of both page and
> skb variables (routines such as give_pages get page *
> so they will accept void* just as happily) or
> move initialization or even use of these variables to
> where they are used.
Ok.
> So above you override skb that you initialized
> at the start of function. It would be better
> to do in the 3'd case:
> skb = buf;
> as well. And then it would be clear why
> " Only for mergeable and big" comment below
> is true.
Ok.
> > + }
> >
> > - err = pskb_trim(skb, len);
> > - if (err) {
> > - pr_debug("%s: pskb_trim failed %i %d\n",
> dev->name,
> > - len, err);
> > - dev->stats.rx_dropped++;
> > - goto drop;
> > - }
> > + if (unlikely(!skb)) {
> > + dev->stats.rx_dropped++;
> > + /* only for mergeable buf and big packets */
> > + give_pages(vi, page);
> > + return;
>
> Did you remove drop: label? If no, is it unused now?
The label is used when checking buf len, but I will remove drop in the
update patch.
> > + void *buf = NULL;
>
> Does this have to be initialized? If not (as it seems) better not do
> it.
I remembered there was a compile warning, I will double check.
> > + freed = vi->rvq->vq_ops->destroy_bufs(vi->rvq,
> virtio_net_free_pages);
>
> This looks like double free to me: should not you remove code that
> does
> __skb_dequeue on recv above?
Nope, this is not a double free, the queue recv skb is still there for
small packet size. But I will remove it in the update patch.
Thanks
Shirley
--
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
