Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi

[Koichiro Den <den@klaipeden.com>]

On Sun, 2017-08-20 at 16:49 -0400, Willem de Bruijn wrote:
> On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den <den@klaipeden.com> wrote:
> > Facing the possible unbounded delay relying on freeing on xmit path,
> > we also better to invoke and clear the upper layer zerocopy callback
> > beforehand to keep them from waiting for unbounded duration in vain.
> 
> Good point.
> 
> > For instance, this removes the possible deadlock in the case that the
> > upper layer is a zerocopy-enabled vhost-net.
> > This does not apply if napi_tx is enabled since it will be called in
> > reasonale time.
> 
> Indeed. Btw, I am gathering data to eventually make napi the default
> mode. But that is taking some time.
I'm looking forward to it. Btw I'm just guessing somehow delayed napi disabling
seems to relieve the interrupt costs much, though it might need to be stateful
so sounds a bit offensive. That's to say, not-yet-used ones are not necessarily
going to be used in the near future, so we have to limit the delay with some
sort of counter. Just guessing as a virtio novice, so pls take it with a grain
of salt ;)
> 
> > 
> > Signed-off-by: Koichiro Den <den@klaipeden.com>
> > ---
> >  drivers/net/virtio_net.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4302f313d9a7..f7deaa5b7b50 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> > 
> >         /* Don't wait up for transmitted skbs to be freed. */
> >         if (!use_napi) {
> > +               if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> > +                       struct ubuf_info *uarg;
> > +                       uarg = skb_shinfo(skb)->destructor_arg;
> > +                       if (uarg->callback)
> > +                           uarg->callback(uarg, true);
> > +                       skb_shinfo(skb)->destructor_arg = NULL;
> > +                       skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> > +               }
> 
> Instead of open coding, this can use skb_zcopy_clear.
I didn't notice it, seems necessary and sufficient. Please let me repost.
Thank you.
> 
> >                 skb_orphan(skb);
> >                 nf_reset(skb);
> >         }
> > --
> > 2.9.4
> > 
> >