Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
Den fre 10 aug. 2018 kl 18:26 skrev Jakub Kicinski : > > On Fri, 10 Aug 2018 17:16:45 +0200, Björn Töpel wrote: > > Den fre 10 aug. 2018 kl 16:10 skrev Daniel Borkmann : > > > > > > On 08/10/2018 11:28 AM, Björn Töpel wrote: > > > > From: Björn Töpel > > > > > > > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > > > > > > > The reverted commit adds a WARN to check against NULL entries in the > > > > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > > > > driver) fast path is required to make a paired > > > > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > > > > addition, a driver using a different allocation scheme than the > > > > default MEM_TYPE_PAGE_SHARED is required to additionally call > > > > xdp_rxq_info_reg_mem_model. > > > > > > > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > > > > that the mem_id_ht rhashtable has a properly inserted allocator id. If > > > > not, this would be a driver bug. A NULL pointer kernel OOPS is > > > > preferred to the WARN. > > > > > > > > Suggested-by: Jesper Dangaard Brouer > > > > Signed-off-by: Björn Töpel > > > > > > Given the last bpf pr went out yesterday night, I've applied this to > > > bpf-next (worst case we can just route it via stable), thanks! > > > > Ah, right! Thanks! > > > > bpf-next is OK. (Since this path is currently not used yet by any driver... > > :-() > > Wasn't this dead code, anyway? The frame return path is for redirects, > and one can't convert_to_xdp_frame presently? Indeed, dead it is. Hmm, I'll remove it as part of the i40e zc submission. Björn
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
On Fri, 10 Aug 2018 17:16:45 +0200, Björn Töpel wrote: > Den fre 10 aug. 2018 kl 16:10 skrev Daniel Borkmann : > > > > On 08/10/2018 11:28 AM, Björn Töpel wrote: > > > From: Björn Töpel > > > > > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > > > > > The reverted commit adds a WARN to check against NULL entries in the > > > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > > > driver) fast path is required to make a paired > > > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > > > addition, a driver using a different allocation scheme than the > > > default MEM_TYPE_PAGE_SHARED is required to additionally call > > > xdp_rxq_info_reg_mem_model. > > > > > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > > > that the mem_id_ht rhashtable has a properly inserted allocator id. If > > > not, this would be a driver bug. A NULL pointer kernel OOPS is > > > preferred to the WARN. > > > > > > Suggested-by: Jesper Dangaard Brouer > > > Signed-off-by: Björn Töpel > > > > Given the last bpf pr went out yesterday night, I've applied this to > > bpf-next (worst case we can just route it via stable), thanks! > > Ah, right! Thanks! > > bpf-next is OK. (Since this path is currently not used yet by any driver... > :-() Wasn't this dead code, anyway? The frame return path is for redirects, and one can't convert_to_xdp_frame presently?
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
On Fri, 10 Aug 2018 17:15:07 +0200, Björn Töpel wrote: > Den fre 10 aug. 2018 kl 12:18 skrev Jesper Dangaard Brouer > : > > > > On Fri, 10 Aug 2018 11:28:02 +0200 > > Björn Töpel wrote: > > > > > From: Björn Töpel > > > > > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > > > > > The reverted commit adds a WARN to check against NULL entries in the > > > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > > > driver) fast path is required to make a paired > > > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > > > addition, a driver using a different allocation scheme than the > > > default MEM_TYPE_PAGE_SHARED is required to additionally call > > > xdp_rxq_info_reg_mem_model. > > > > > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > > > that the mem_id_ht rhashtable has a properly inserted allocator id. If > > > not, this would be a driver bug. A NULL pointer kernel OOPS is > > > preferred to the WARN. > > > > Acked-by: Jesper Dangaard Brouer > > > > As a comment says in the code: /* NB! Only valid from an xdp_buff! */ > > Which is (currently) guarded by the return/exit in convert_to_xdp_frame(). > > > > This means that this code path can only be invoked while the driver is > > still running under the RX NAPI process. Thus, there is no chance that > > the allocator-id is gone (via calling xdp_rxq_info_unreg) for this code > > path. > > > > But I really hope we at somepoint can convert a MEM_TYPE_ZERO_COPY into > > a form of xdp_frame, that can travel further into the redirect-core. > > In which case, we likely need to handle the NULL case (but also need > > other code to handle what to do with the memory backing the frame) > > > > (I'm my vision here:) > > > > I really dislike that the current Zero-Copy mode steal ALL packets, > > when ZC is enabled on a RX-queue. This is not better than the existing > > bypass solutions, which have ugly ways of re-injecting packet back into > > the network stack. With the integration with XDP, we have the > > flexibility of selecting frames, that we don't want to be "bypassed" > > into AF_XDP, and want the kernel process these. (The most common > > use-case is letting the kernel handle the arptable). IHMO this is what > > will/would make AF_XDP superior to other bypass solutions. Perhaps I'm misunderstanding, but I don't think that's necessarily true. AFAIU on XDP_PASS drivers should copy the frame into a skb and pass it up the stack. Granted that's fairly slow but *semantically* AF_XDP doesn't necessarily steal all the packets :) > Thanks for putting your visions/ideas here! I agree with both of your > last sections, and this is what we're working towards. AF_XDP ZC has > to play nicer with XDP. The current (well, the soon-to-be-published > [1] ;-)) ZC scheme is just a first step, and should be seen as a > starting point so people can start playing using AF_XDP. Jakub also > mentioned these issues a couple of threads ago, so there are > definitely more people feeling the ZC allocator pains. Mid-term a > sophisticated/proper and generic (for inter-driver reuse) ZC allocator > is needed; Converting xdp_buffs to xdp_frames cheaply for multi-CPU > completion, and hopefully dito for the XDP_PASS/kernel stack path. But > let's start with something simple that works, and take it from there. > > Björn > > [1] WIP: https://github.com/bjoto/linux/tree/af-xdp-i40e-zc Nice, looking forward to a refresh of i40e patches! :) > > > Suggested-by: Jesper Dangaard Brouer > > > Signed-off-by: Björn Töpel > > > --- > > > net/core/xdp.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c > > > index 6771f1855b96..9d1f22072d5d 100644 > > > --- a/net/core/xdp.c > > > +++ b/net/core/xdp.c > > > @@ -345,8 +345,7 @@ static void __xdp_return(void *data, struct > > > xdp_mem_info *mem, bool napi_direct, > > > rcu_read_lock(); > > > /* mem->id is valid, checked in > > > xdp_rxq_info_reg_mem_model() */ > > > xa = rhashtable_lookup(mem_id_ht, >id, > > > mem_id_rht_params); > > > - if (!WARN_ON_ONCE(!xa)) > > > - xa->zc_alloc->free(xa->zc_alloc, handle); > > > + xa->zc_alloc->free(xa->zc_alloc, handle); > > > rcu_read_unlock(); > > > default: > > > /* Not possible, checked in xdp_rxq_info_reg_mem_model() */ > > > > > > > > > > > -- > > Best regards, > > Jesper Dangaard Brouer > > MSc.CS, Principal Kernel Engineer at Red Hat > > LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
Den fre 10 aug. 2018 kl 16:10 skrev Daniel Borkmann : > > On 08/10/2018 11:28 AM, Björn Töpel wrote: > > From: Björn Töpel > > > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > > > The reverted commit adds a WARN to check against NULL entries in the > > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > > driver) fast path is required to make a paired > > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > > addition, a driver using a different allocation scheme than the > > default MEM_TYPE_PAGE_SHARED is required to additionally call > > xdp_rxq_info_reg_mem_model. > > > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > > that the mem_id_ht rhashtable has a properly inserted allocator id. If > > not, this would be a driver bug. A NULL pointer kernel OOPS is > > preferred to the WARN. > > > > Suggested-by: Jesper Dangaard Brouer > > Signed-off-by: Björn Töpel > > Given the last bpf pr went out yesterday night, I've applied this to > bpf-next (worst case we can just route it via stable), thanks! Ah, right! Thanks! bpf-next is OK. (Since this path is currently not used yet by any driver... :-() Björn
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
Den fre 10 aug. 2018 kl 12:18 skrev Jesper Dangaard Brouer : > > On Fri, 10 Aug 2018 11:28:02 +0200 > Björn Töpel wrote: > > > From: Björn Töpel > > > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > > > The reverted commit adds a WARN to check against NULL entries in the > > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > > driver) fast path is required to make a paired > > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > > addition, a driver using a different allocation scheme than the > > default MEM_TYPE_PAGE_SHARED is required to additionally call > > xdp_rxq_info_reg_mem_model. > > > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > > that the mem_id_ht rhashtable has a properly inserted allocator id. If > > not, this would be a driver bug. A NULL pointer kernel OOPS is > > preferred to the WARN. > > Acked-by: Jesper Dangaard Brouer > > As a comment says in the code: /* NB! Only valid from an xdp_buff! */ > Which is (currently) guarded by the return/exit in convert_to_xdp_frame(). > > This means that this code path can only be invoked while the driver is > still running under the RX NAPI process. Thus, there is no chance that > the allocator-id is gone (via calling xdp_rxq_info_unreg) for this code > path. > > But I really hope we at somepoint can convert a MEM_TYPE_ZERO_COPY into > a form of xdp_frame, that can travel further into the redirect-core. > In which case, we likely need to handle the NULL case (but also need > other code to handle what to do with the memory backing the frame) > > (I'm my vision here:) > > I really dislike that the current Zero-Copy mode steal ALL packets, > when ZC is enabled on a RX-queue. This is not better than the existing > bypass solutions, which have ugly ways of re-injecting packet back into > the network stack. With the integration with XDP, we have the > flexibility of selecting frames, that we don't want to be "bypassed" > into AF_XDP, and want the kernel process these. (The most common > use-case is letting the kernel handle the arptable). IHMO this is what > will/would make AF_XDP superior to other bypass solutions. > > Thanks for putting your visions/ideas here! I agree with both of your last sections, and this is what we're working towards. AF_XDP ZC has to play nicer with XDP. The current (well, the soon-to-be-published [1] ;-)) ZC scheme is just a first step, and should be seen as a starting point so people can start playing using AF_XDP. Jakub also mentioned these issues a couple of threads ago, so there are definitely more people feeling the ZC allocator pains. Mid-term a sophisticated/proper and generic (for inter-driver reuse) ZC allocator is needed; Converting xdp_buffs to xdp_frames cheaply for multi-CPU completion, and hopefully dito for the XDP_PASS/kernel stack path. But let's start with something simple that works, and take it from there. Björn [1] WIP: https://github.com/bjoto/linux/tree/af-xdp-i40e-zc > > Suggested-by: Jesper Dangaard Brouer > > Signed-off-by: Björn Töpel > > --- > > net/core/xdp.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c > > index 6771f1855b96..9d1f22072d5d 100644 > > --- a/net/core/xdp.c > > +++ b/net/core/xdp.c > > @@ -345,8 +345,7 @@ static void __xdp_return(void *data, struct > > xdp_mem_info *mem, bool napi_direct, > > rcu_read_lock(); > > /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() > > */ > > xa = rhashtable_lookup(mem_id_ht, >id, > > mem_id_rht_params); > > - if (!WARN_ON_ONCE(!xa)) > > - xa->zc_alloc->free(xa->zc_alloc, handle); > > + xa->zc_alloc->free(xa->zc_alloc, handle); > > rcu_read_unlock(); > > default: > > /* Not possible, checked in xdp_rxq_info_reg_mem_model() */ > > > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
On 08/10/2018 11:28 AM, Björn Töpel wrote: > From: Björn Töpel > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > The reverted commit adds a WARN to check against NULL entries in the > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > driver) fast path is required to make a paired > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > addition, a driver using a different allocation scheme than the > default MEM_TYPE_PAGE_SHARED is required to additionally call > xdp_rxq_info_reg_mem_model. > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > that the mem_id_ht rhashtable has a properly inserted allocator id. If > not, this would be a driver bug. A NULL pointer kernel OOPS is > preferred to the WARN. > > Suggested-by: Jesper Dangaard Brouer > Signed-off-by: Björn Töpel Given the last bpf pr went out yesterday night, I've applied this to bpf-next (worst case we can just route it via stable), thanks!
Re: [PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
On Fri, 10 Aug 2018 11:28:02 +0200 Björn Töpel wrote: > From: Björn Töpel > > This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. > > The reverted commit adds a WARN to check against NULL entries in the > mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or > driver) fast path is required to make a paired > xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In > addition, a driver using a different allocation scheme than the > default MEM_TYPE_PAGE_SHARED is required to additionally call > xdp_rxq_info_reg_mem_model. > > For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures > that the mem_id_ht rhashtable has a properly inserted allocator id. If > not, this would be a driver bug. A NULL pointer kernel OOPS is > preferred to the WARN. Acked-by: Jesper Dangaard Brouer As a comment says in the code: /* NB! Only valid from an xdp_buff! */ Which is (currently) guarded by the return/exit in convert_to_xdp_frame(). This means that this code path can only be invoked while the driver is still running under the RX NAPI process. Thus, there is no chance that the allocator-id is gone (via calling xdp_rxq_info_unreg) for this code path. But I really hope we at somepoint can convert a MEM_TYPE_ZERO_COPY into a form of xdp_frame, that can travel further into the redirect-core. In which case, we likely need to handle the NULL case (but also need other code to handle what to do with the memory backing the frame) (I'm my vision here:) I really dislike that the current Zero-Copy mode steal ALL packets, when ZC is enabled on a RX-queue. This is not better than the existing bypass solutions, which have ugly ways of re-injecting packet back into the network stack. With the integration with XDP, we have the flexibility of selecting frames, that we don't want to be "bypassed" into AF_XDP, and want the kernel process these. (The most common use-case is letting the kernel handle the arptable). IHMO this is what will/would make AF_XDP superior to other bypass solutions. > Suggested-by: Jesper Dangaard Brouer > Signed-off-by: Björn Töpel > --- > net/core/xdp.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 6771f1855b96..9d1f22072d5d 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -345,8 +345,7 @@ static void __xdp_return(void *data, struct xdp_mem_info > *mem, bool napi_direct, > rcu_read_lock(); > /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */ > xa = rhashtable_lookup(mem_id_ht, >id, mem_id_rht_params); > - if (!WARN_ON_ONCE(!xa)) > - xa->zc_alloc->free(xa->zc_alloc, handle); > + xa->zc_alloc->free(xa->zc_alloc, handle); > rcu_read_unlock(); > default: > /* Not possible, checked in xdp_rxq_info_reg_mem_model() */ -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
[PATCH bpf] Revert "xdp: add NULL pointer check in __xdp_return()"
From: Björn Töpel This reverts commit 36e0f12bbfd3016f495904b35e41c5711707509f. The reverted commit adds a WARN to check against NULL entries in the mem_id_ht rhashtable. Any kernel path implementing the XDP (generic or driver) fast path is required to make a paired xdp_rxq_info_reg/xdp_rxq_info_unreg call for proper function. In addition, a driver using a different allocation scheme than the default MEM_TYPE_PAGE_SHARED is required to additionally call xdp_rxq_info_reg_mem_model. For MEM_TYPE_ZERO_COPY, an xdp_rxq_info_reg_mem_model call ensures that the mem_id_ht rhashtable has a properly inserted allocator id. If not, this would be a driver bug. A NULL pointer kernel OOPS is preferred to the WARN. Suggested-by: Jesper Dangaard Brouer Signed-off-by: Björn Töpel --- net/core/xdp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/core/xdp.c b/net/core/xdp.c index 6771f1855b96..9d1f22072d5d 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -345,8 +345,7 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct, rcu_read_lock(); /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */ xa = rhashtable_lookup(mem_id_ht, >id, mem_id_rht_params); - if (!WARN_ON_ONCE(!xa)) - xa->zc_alloc->free(xa->zc_alloc, handle); + xa->zc_alloc->free(xa->zc_alloc, handle); rcu_read_unlock(); default: /* Not possible, checked in xdp_rxq_info_reg_mem_model() */ -- 2.17.1
