| Message ID | 20210329080039.32753-1-boon.leong.ong@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [net,1/1] xdp: fix xdp_return_frame() kernel BUG throw for page_pool memory model | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: yhs@fb.com kpsingh@kernel.org andrii@kernel.org kafai@fb.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Mon, 29 Mar 2021 16:00:39 +0800 Ong Boon Leong <boon.leong.ong@intel.com> wrote: > xdp_return_frame() may be called outside of NAPI context to return > xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with > napi_direct = false. For page_pool memory model, __xdp_return() calls > xdp_return_frame_no_direct() unconditionally and below false negative > kernel BUG throw happened under preempt-rt build: > > [ 430.450355] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3884 > [ 430.451678] caller is __xdp_return+0x1ff/0x2e0 > [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E 5.12.0-rc2+ #45 > > So, this patch fixes the issue by adding "if (napi_direct)" condition > to skip calling xdp_return_frame_no_direct() if napi_direct = false. > > Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of xdp_return_frame") > Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com> > --- This looks correct to me. Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > net/core/xdp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 05354976c1fc..4eaa28972af2 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -350,7 +350,8 @@ 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); > - napi_direct &= !xdp_return_frame_no_direct(); > + if (napi_direct) > + napi_direct &= !xdp_return_frame_no_direct(); if (napi_direct && xdp_return_frame_no_direct()) napi_direct = false; I wonder if this code would be easier to understand? > page_pool_put_full_page(xa->page_pool, page, napi_direct); > rcu_read_unlock(); > break;
Jesper Dangaard Brouer <brouer@redhat.com> writes: > On Mon, 29 Mar 2021 16:00:39 +0800 > Ong Boon Leong <boon.leong.ong@intel.com> wrote: > >> xdp_return_frame() may be called outside of NAPI context to return >> xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with >> napi_direct = false. For page_pool memory model, __xdp_return() calls >> xdp_return_frame_no_direct() unconditionally and below false negative >> kernel BUG throw happened under preempt-rt build: >> >> [ 430.450355] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3884 >> [ 430.451678] caller is __xdp_return+0x1ff/0x2e0 >> [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E 5.12.0-rc2+ #45 >> >> So, this patch fixes the issue by adding "if (napi_direct)" condition >> to skip calling xdp_return_frame_no_direct() if napi_direct = false. >> >> Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of xdp_return_frame") >> Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com> >> --- > > This looks correct to me. > > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > > >> net/core/xdp.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index 05354976c1fc..4eaa28972af2 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -350,7 +350,8 @@ 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); >> - napi_direct &= !xdp_return_frame_no_direct(); >> + if (napi_direct) >> + napi_direct &= !xdp_return_frame_no_direct(); > > if (napi_direct && xdp_return_frame_no_direct()) > napi_direct = false; > > I wonder if this code would be easier to understand? Yes, IMO it would! :) -Toke
Hi Ong, Patch is in "Changes Requested". Thus, please send a V2 patch with suggested code changes below. --Jesper On Mon, 29 Mar 2021 18:25:08 +0200 Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Jesper Dangaard Brouer <brouer@redhat.com> writes: > > > On Mon, 29 Mar 2021 16:00:39 +0800 > > Ong Boon Leong <boon.leong.ong@intel.com> wrote: > > > >> xdp_return_frame() may be called outside of NAPI context to return > >> xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with > >> napi_direct = false. For page_pool memory model, __xdp_return() calls > >> xdp_return_frame_no_direct() unconditionally and below false negative > >> kernel BUG throw happened under preempt-rt build: > >> > >> [ 430.450355] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3884 > >> [ 430.451678] caller is __xdp_return+0x1ff/0x2e0 > >> [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E 5.12.0-rc2+ #45 > >> > >> So, this patch fixes the issue by adding "if (napi_direct)" condition > >> to skip calling xdp_return_frame_no_direct() if napi_direct = false. > >> > >> Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of xdp_return_frame") > >> Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com> > >> --- > > > > This looks correct to me. > > > > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > > > > > >> net/core/xdp.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/core/xdp.c b/net/core/xdp.c > >> index 05354976c1fc..4eaa28972af2 100644 > >> --- a/net/core/xdp.c > >> +++ b/net/core/xdp.c > >> @@ -350,7 +350,8 @@ 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); > >> - napi_direct &= !xdp_return_frame_no_direct(); > >> + if (napi_direct) > >> + napi_direct &= !xdp_return_frame_no_direct(); > > > > if (napi_direct && xdp_return_frame_no_direct()) > > napi_direct = false; > > > > I wonder if this code would be easier to understand? > > Yes, IMO it would! :) > > -Toke >
>> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index 05354976c1fc..4eaa28972af2 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -350,7 +350,8 @@ 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); >> - napi_direct &= !xdp_return_frame_no_direct(); >> + if (napi_direct) >> + napi_direct &= !xdp_return_frame_no_direct(); > >if (napi_direct && xdp_return_frame_no_direct()) > napi_direct = false; > >I wonder if this code would be easier to understand? > Ya, I think this is more readable. Thanks. I will send v2.
diff --git a/net/core/xdp.c b/net/core/xdp.c index 05354976c1fc..4eaa28972af2 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -350,7 +350,8 @@ 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); - napi_direct &= !xdp_return_frame_no_direct(); + if (napi_direct) + napi_direct &= !xdp_return_frame_no_direct(); page_pool_put_full_page(xa->page_pool, page, napi_direct); rcu_read_unlock(); break;
xdp_return_frame() may be called outside of NAPI context to return xdpf back to page_pool. xdp_return_frame() calls __xdp_return() with napi_direct = false. For page_pool memory model, __xdp_return() calls xdp_return_frame_no_direct() unconditionally and below false negative kernel BUG throw happened under preempt-rt build: [ 430.450355] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3884 [ 430.451678] caller is __xdp_return+0x1ff/0x2e0 [ 430.452111] CPU: 0 PID: 3884 Comm: modprobe Tainted: G U E 5.12.0-rc2+ #45 So, this patch fixes the issue by adding "if (napi_direct)" condition to skip calling xdp_return_frame_no_direct() if napi_direct = false. Fixes: 2539650fadbf ("xdp: Helpers for disabling napi_direct of xdp_return_frame") Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com> --- net/core/xdp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)