| Message ID | 20210706174409.15001-1-vjsanjay@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/runqslower: use __state instead of state | expand |
On 7/6/21 10:44 AM, SanjayKumar J wrote: > task->state is renamed to task->__state in task_struct Could you add a reference to 2f064a59a11f ("sched: Change task_struct::state") which added this change? I think this should go to bpf tree as the change is in linus tree now. Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")? Please align comments to the left without margins. > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> This Singed-off-by is not needed. You can add my Ack in the next revision: Acked-by: Yonghong Song <yhs@fb.com> > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > --- > tools/bpf/runqslower/runqslower.bpf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c > index 645530ca7e98..ab9353f2fd46 100644 > --- a/tools/bpf/runqslower/runqslower.bpf.c > +++ b/tools/bpf/runqslower/runqslower.bpf.c > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) > u32 pid; > > /* ivcsw: treat like an enqueue event and store timestamp */ > - if (prev->state == TASK_RUNNING) > + if (prev->__state == TASK_RUNNING) Currently, runqslower.bpf.c uses vmlinux.h. I am thinking to use bpf_core_field_exists(), but we need to single out task_struct structure from vmlinux.h with both state and __state fields, we could make it work by *changes* like #define task_struct task_struct_orig #include "vmlinux.h" #undef task_struct struct task_struct { ... state; ... __state; ... }; Considering tools/bpf/runqslower is tied with a particular kernel source, and vmlinux.h mostly derived from that kernel source, I feel the above change is not necessary. > trace_enqueue(prev); > > pid = next->pid; >
On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 7/6/21 10:44 AM, SanjayKumar J wrote: > > task->state is renamed to task->__state in task_struct > > Could you add a reference to > 2f064a59a11f ("sched: Change task_struct::state") > which added this change? > > I think this should go to bpf tree as the change is in linus tree now. > Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")? > > Please align comments to the left without margins. > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > This Singed-off-by is not needed. > > You can add my Ack in the next revision: > Acked-by: Yonghong Song <yhs@fb.com> > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > --- > > tools/bpf/runqslower/runqslower.bpf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c > > index 645530ca7e98..ab9353f2fd46 100644 > > --- a/tools/bpf/runqslower/runqslower.bpf.c > > +++ b/tools/bpf/runqslower/runqslower.bpf.c > > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) > > u32 pid; > > > > /* ivcsw: treat like an enqueue event and store timestamp */ > > - if (prev->state == TASK_RUNNING) > > + if (prev->__state == TASK_RUNNING) > > Currently, runqslower.bpf.c uses vmlinux.h. > I am thinking to use bpf_core_field_exists(), but we need to > single out task_struct structure from vmlinux.h > with both state and __state fields, we could make it work > by *changes* like > > #define task_struct task_struct_orig > #include "vmlinux.h" > #undef task_struct > > struct task_struct { > ... state; > ... __state; > ... > }; no need for such surgery, recommended way is to use ___suffix to declare incompatible struct definition: struct task_struct___old { int state; }; Then do casting in BPF code. We don't have to do it in kernel tree's runqslower, but we'll definitely have to do that for libbpf-tools' runqslower and runqlat. > > Considering tools/bpf/runqslower is tied with a particular > kernel source, and vmlinux.h mostly derived from that > kernel source, I feel the above change is not necessary. > > > trace_enqueue(prev); > > > > pid = next->pid; > >
On Tue, Jul 6, 2021 at 4:11 PM Song Liu <song@kernel.org> wrote: > > On Tue, Jul 6, 2021 at 3:05 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > > > > > On 7/6/21 10:44 AM, SanjayKumar J wrote: > > > > task->state is renamed to task->__state in task_struct > > > > > > Could you add a reference to > > > 2f064a59a11f ("sched: Change task_struct::state") > > > which added this change? > > > > > > I think this should go to bpf tree as the change is in linus tree now. > > > Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")? > > > > > > Please align comments to the left without margins. > > > > > > > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > > > > > This Singed-off-by is not needed. > > > > > > You can add my Ack in the next revision: > > > Acked-by: Yonghong Song <yhs@fb.com> > > > > > > > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > > > --- > > > > tools/bpf/runqslower/runqslower.bpf.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c > > > > index 645530ca7e98..ab9353f2fd46 100644 > > > > --- a/tools/bpf/runqslower/runqslower.bpf.c > > > > +++ b/tools/bpf/runqslower/runqslower.bpf.c > > > > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) > > > > u32 pid; > > > > > > > > /* ivcsw: treat like an enqueue event and store timestamp */ > > > > - if (prev->state == TASK_RUNNING) > > > > + if (prev->__state == TASK_RUNNING) > > > > > > Currently, runqslower.bpf.c uses vmlinux.h. > > > I am thinking to use bpf_core_field_exists(), but we need to > > > single out task_struct structure from vmlinux.h > > > with both state and __state fields, we could make it work > > > by *changes* like > > > > > > #define task_struct task_struct_orig > > > #include "vmlinux.h" > > > #undef task_struct > > > > > > struct task_struct { > > > ... state; > > > ... __state; > > > ... > > > }; > > > > > > no need for such surgery, recommended way is to use ___suffix to > > declare incompatible struct definition: > > > > struct task_struct___old { > > int state; > > }; > > > > Then do casting in BPF code. We don't have to do it in kernel tree's > > runqslower, but we'll definitely have to do that for libbpf-tools' > > runqslower and runqlat. > > Question on this topic: state and __state are of different sizes here. IIUC, > bpf_core_types_are_compat() does allow size mismatch. But it may cause > problems in some cases, no? For example, would some combination make > task->state return 32 extra bits from another field and cause confusion? In this case it's two different fields, long state vs int __state, so there is no confusion, you'd be using either one or another. But even if it was the same field and its type changed from long to int, libbpf will still try to accommodate that. Worst case, BPF_CORE_READ_BITFIELD() is able to read any bitfield or integer field, regardless of its size. > > Thanks, > Song
On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 7/6/21 10:44 AM, SanjayKumar J wrote: > > task->state is renamed to task->__state in task_struct > > Could you add a reference to > 2f064a59a11f ("sched: Change task_struct::state") > which added this change? > > I think this should go to bpf tree as the change is in linus tree now. > Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")? > > Please align comments to the left without margins. > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > This Singed-off-by is not needed. > > You can add my Ack in the next revision: > Acked-by: Yonghong Song <yhs@fb.com> > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> Please use a full and proper name in Signed-off-by. This currently breaks selftests build in bpf tree, so please prioritize sending v2. Alternatively, we can apply Jiri's fix instead. > > --- > > tools/bpf/runqslower/runqslower.bpf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c > > index 645530ca7e98..ab9353f2fd46 100644 > > --- a/tools/bpf/runqslower/runqslower.bpf.c > > +++ b/tools/bpf/runqslower/runqslower.bpf.c > > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) > > u32 pid; > > > > /* ivcsw: treat like an enqueue event and store timestamp */ > > - if (prev->state == TASK_RUNNING) > > + if (prev->__state == TASK_RUNNING) > > Currently, runqslower.bpf.c uses vmlinux.h. > I am thinking to use bpf_core_field_exists(), but we need to > single out task_struct structure from vmlinux.h > with both state and __state fields, we could make it work > by *changes* like > > #define task_struct task_struct_orig > #include "vmlinux.h" > #undef task_struct > > struct task_struct { > ... state; > ... __state; > ... > }; > > Considering tools/bpf/runqslower is tied with a particular > kernel source, and vmlinux.h mostly derived from that > kernel source, I feel the above change is not necessary. > > > trace_enqueue(prev); > > > > pid = next->pid; > >
On Wed, Jul 7, 2021 at 9:53 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Jul 6, 2021 at 11:26 AM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 7/6/21 10:44 AM, SanjayKumar J wrote: > > > task->state is renamed to task->__state in task_struct > > > > Could you add a reference to > > 2f064a59a11f ("sched: Change task_struct::state") > > which added this change? > > > > I think this should go to bpf tree as the change is in linus tree now. > > Could you annotate the tag as "[PATCH bpf]" ("[PATCH bpf v2]")? > > > > Please align comments to the left without margins. > > > > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > > > This Singed-off-by is not needed. > > > > You can add my Ack in the next revision: > > Acked-by: Yonghong Song <yhs@fb.com> > > > > > > > > Signed-off-by: SanjayKumar J <vjsanjay@gmail.com> > > Please use a full and proper name in Signed-off-by. > > This currently breaks selftests build in bpf tree, so please > prioritize sending v2. Alternatively, we can apply Jiri's fix instead. > I have send v4 .. apologies for the iterations. > > > --- > > > tools/bpf/runqslower/runqslower.bpf.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c > > > index 645530ca7e98..ab9353f2fd46 100644 > > > --- a/tools/bpf/runqslower/runqslower.bpf.c > > > +++ b/tools/bpf/runqslower/runqslower.bpf.c > > > @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) > > > u32 pid; > > > > > > /* ivcsw: treat like an enqueue event and store timestamp */ > > > - if (prev->state == TASK_RUNNING) > > > + if (prev->__state == TASK_RUNNING) > > > > Currently, runqslower.bpf.c uses vmlinux.h. > > I am thinking to use bpf_core_field_exists(), but we need to > > single out task_struct structure from vmlinux.h > > with both state and __state fields, we could make it work > > by *changes* like > > > > #define task_struct task_struct_orig > > #include "vmlinux.h" > > #undef task_struct > > > > struct task_struct { > > ... state; > > ... __state; > > ... > > }; > > > > Considering tools/bpf/runqslower is tied with a particular > > kernel source, and vmlinux.h mostly derived from that > > kernel source, I feel the above change is not necessary. > > > > > trace_enqueue(prev); > > > > > > pid = next->pid; > > >
diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c index 645530ca7e98..ab9353f2fd46 100644 --- a/tools/bpf/runqslower/runqslower.bpf.c +++ b/tools/bpf/runqslower/runqslower.bpf.c @@ -74,7 +74,7 @@ int handle__sched_switch(u64 *ctx) u32 pid; /* ivcsw: treat like an enqueue event and store timestamp */ - if (prev->state == TASK_RUNNING) + if (prev->__state == TASK_RUNNING) trace_enqueue(prev); pid = next->pid;