diff mbox series

[v3,bpf-next,6/8] xdp: Add a flag for disabling napi_direct of xdp_return_frame in xdp_mem_info

Message ID 20180722151308.5480-7-toshiaki.makita1@gmail.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series veth: Driver XDP | expand

Commit Message

Toshiaki Makita July 22, 2018, 3:13 p.m. UTC 
From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

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 <makita.toshiaki@lab.ntt.co.jp>
---
 include/net/xdp.h | 4 ++++
 net/core/xdp.c    | 6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski July 24, 2018, 1:22 a.m. UTC | #1 
On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> 
> 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 <makita.toshiaki@lab.ntt.co.jp>

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.

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

> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index fcb033f51d8c..1d1bc6553ff2 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -41,6 +41,9 @@ enum xdp_mem_type {
>  	MEM_TYPE_MAX,
>  };
>  
> +/* XDP flags for xdp_mem_info */
> +#define XDP_MEM_RF_NO_DIRECT	BIT(0)	/* don't use napi_direct */
> +
>  /* XDP flags for ndo_xdp_xmit */
>  #define XDP_XMIT_FLUSH		(1U << 0)	/* doorbell signal consumer */
>  #define XDP_XMIT_FLAGS_MASK	XDP_XMIT_FLUSH
> @@ -48,6 +51,7 @@ enum xdp_mem_type {
>  struct xdp_mem_info {
>  	u32 type; /* enum xdp_mem_type, but known size type */
>  	u32 id;
> +	u32 flags;
>  };
>  
>  struct page_pool;
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 57285383ed00..1426c608fd75 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -330,10 +330,12 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>  		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>  		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>  		page = virt_to_head_page(data);
> -		if (xa)
> +		if (xa) {
> +			napi_direct &= !(mem->flags & XDP_MEM_RF_NO_DIRECT);
>  			page_pool_put_page(xa->page_pool, page, napi_direct);
> -		else
> +		} else {
>  			put_page(page);
> +		}
>  		rcu_read_unlock();
>  		break;
>  	case MEM_TYPE_PAGE_SHARED:
Toshiaki Makita July 24, 2018, 2:43 a.m. UTC | #2 
On 2018/07/24 10:22, Jakub Kicinski wrote:
> On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:
>> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>
>> 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 <makita.toshiaki@lab.ntt.co.jp>
> 
> 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.

> 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...)

> 
>> diff --git a/include/net/xdp.h b/include/net/xdp.h
>> index fcb033f51d8c..1d1bc6553ff2 100644
>> --- a/include/net/xdp.h
>> +++ b/include/net/xdp.h
>> @@ -41,6 +41,9 @@ enum xdp_mem_type {
>>  	MEM_TYPE_MAX,
>>  };
>>  
>> +/* XDP flags for xdp_mem_info */
>> +#define XDP_MEM_RF_NO_DIRECT	BIT(0)	/* don't use napi_direct */
>> +
>>  /* XDP flags for ndo_xdp_xmit */
>>  #define XDP_XMIT_FLUSH		(1U << 0)	/* doorbell signal consumer */
>>  #define XDP_XMIT_FLAGS_MASK	XDP_XMIT_FLUSH
>> @@ -48,6 +51,7 @@ enum xdp_mem_type {
>>  struct xdp_mem_info {
>>  	u32 type; /* enum xdp_mem_type, but known size type */
>>  	u32 id;
>> +	u32 flags;
>>  };
>>  
>>  struct page_pool;
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 57285383ed00..1426c608fd75 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -330,10 +330,12 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>>  		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>>  		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>>  		page = virt_to_head_page(data);
>> -		if (xa)
>> +		if (xa) {
>> +			napi_direct &= !(mem->flags & XDP_MEM_RF_NO_DIRECT);
>>  			page_pool_put_page(xa->page_pool, page, napi_direct);
>> -		else
>> +		} else {
>>  			put_page(page);
>> +		}
>>  		rcu_read_unlock();
>>  		break;
>>  	case MEM_TYPE_PAGE_SHARED:
> 
> 
>
Jakub Kicinski July 24, 2018, 3:38 a.m. UTC | #3 
On Tue, 24 Jul 2018 11:43:11 +0900, Toshiaki Makita wrote:
> On 2018/07/24 10:22, Jakub Kicinski wrote:
> > On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:  
> >> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> >>
> >> 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 <makita.toshiaki@lab.ntt.co.jp>  
> > 
> > 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.

> > 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 :/
Toshiaki Makita July 24, 2018, 4:02 a.m. UTC | #4 
On 2018/07/24 12:38, Jakub Kicinski wrote:
> On Tue, 24 Jul 2018 11:43:11 +0900, Toshiaki Makita wrote:
>> On 2018/07/24 10:22, Jakub Kicinski wrote:
>>> On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:  
>>>> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>>
>>>> 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 <makita.toshiaki@lab.ntt.co.jp>  
>>>
>>> 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.

OK, let me check if this works.
diff mbox series

Patch

diff --git a/include/net/xdp.h b/include/net/xdp.h
index fcb033f51d8c..1d1bc6553ff2 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -41,6 +41,9 @@  enum xdp_mem_type {
 	MEM_TYPE_MAX,
 };
 
+/* XDP flags for xdp_mem_info */
+#define XDP_MEM_RF_NO_DIRECT	BIT(0)	/* don't use napi_direct */
+
 /* XDP flags for ndo_xdp_xmit */
 #define XDP_XMIT_FLUSH		(1U << 0)	/* doorbell signal consumer */
 #define XDP_XMIT_FLAGS_MASK	XDP_XMIT_FLUSH
@@ -48,6 +51,7 @@  enum xdp_mem_type {
 struct xdp_mem_info {
 	u32 type; /* enum xdp_mem_type, but known size type */
 	u32 id;
+	u32 flags;
 };
 
 struct page_pool;
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 57285383ed00..1426c608fd75 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -330,10 +330,12 @@  static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
 		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
 		page = virt_to_head_page(data);
-		if (xa)
+		if (xa) {
+			napi_direct &= !(mem->flags & XDP_MEM_RF_NO_DIRECT);
 			page_pool_put_page(xa->page_pool, page, napi_direct);
-		else
+		} else {
 			put_page(page);
+		}
 		rcu_read_unlock();
 		break;
 	case MEM_TYPE_PAGE_SHARED: