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

On Tue, 2017-08-22 at 08:11 -0400, Willem de Bruijn wrote:

On Sun, Aug 20, 2017 at 4:49 PM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Sat, Aug 19, 2017 at 2:38 AM, Koichiro Den [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

Signed-off-by: Koichiro Den <redacted>
---
 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.

It is not correct to only send the zerocopy completion here, as
the skb will still be shared with the nic. The only safe approach
to notifying early is to create a copy with skb_orphan_frags_rx.
That will call skb_zcopy_clear(skb, false) internally. But the
copy will be very expensive.

I noticed this email after my last post. I cannot not imagine how it could be
unsafe in virtio case. Sorry could you explain a bit more?

Is the deadlock you refer to the case where tx interrupts are
disabled, so transmit completions are only handled in start_xmit
and somehow the socket(s) are unable to send new data? The
question is what is blocking them. If it is zerocopy, it may be a
too low optmem_max or hitting the per-user locked pages limit.
We may have to keep interrupts enabled when zerocopy skbs
are in flight.

Even if we keep interrupts enabled, We miss the completion without start_xmit.
And it's also likely that the next start_xmit depends on the completion itself
as I described in my last post.