On Tue, 24 Jul 2018 11:43:11 +0900, Toshiaki Makita wrote:

On 2018/07/24 10:22, Jakub Kicinski wrote:

quoted

On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:  

quoted

From: Toshiaki Makita <redacted>

We need some mechanism to disable napi_direct on calling
xdp_return_frame_rx_napi() from some context.
When veth gets support of XDP_REDIRECT, it will redirects packets which
are redirected from other devices. On redirection veth will reuse
xdp_mem_info of the redirection source device to make return_frame work.
But in this case .ndo_xdp_xmit() called from veth redirection uses
xdp_mem_info which is not guarded by NAPI, because the .ndo_xdp_xmit is
not called directly from the rxq which owns the xdp_mem_info.

This approach introduces a flag in xdp_mem_info to indicate that
napi_direct should be disabled even when _rx_napi variant is used.

Signed-off-by: Toshiaki Makita <redacted>  

To be clear - you will modify flags of the original source device if it
ever redirected a frame to a software device like veth?  Seems a bit
heavy handed.  The xdp_return_frame_rx_napi() is only really used on
error paths, but still..  Also as you note the original NAPI can run
concurrently with your veth dest one, but also with NAPIs of other veth
devices, so the non-atomic xdp.rxq->mem.flags |= XDP_MEM_RF_NO_DIRECT;
makes me worried.  

xdp_mem_info is copied in xdp_frame in convert_to_xdp_frame() so the
field is local to the frame. Changing flags affects only the frame.
xdp.rxq is local to NAPI thread, so no worries about atomicity.

Ah, right!  mem_info used to be just 8B, now it would be 12B.
Alternatively we could perhaps add this info to struct redirect_info,
through xdp_do_redirect() to avoid the per-frame cost.  I'm not sure
that's better.

quoted

Would you mind elaborating why not handle the RX completely in the NAPI
context of the original device?  

Originally it was difficult to implement .ndo_xdp_xmit() and
.ndo_xdp_flush() model without creating NAPI in veth. Now it is changed
so I'm not sure how difficult it is at this point.
But in any case I want to avoid stack inflation by veth NAPI. (Imagine
some misconfiguration like calling XDP_TX on both side of veth...)

True :/