Re: [PATCH v2] workqueue: add function in event of workqueue_activate_work
On 2024/3/8 10:23, Steven Rostedt wrote:
On Fri, 8 Mar 2024 10:18:18 +0800
Kassey Li wrote:
The trace event "workqueue_activate_work" only print work struct.
However, function is the region of interest in a full sequence of work.
Current workqueue_activate_work trace event output:
workqueue_activate_work: work struct ff88b4a0f450
With this change, workqueue_activate_work will print the function name,
align with workqueue_queue_work/execute_start/execute_end event.
workqueue_activate_work: work struct ff80413a78b8
function=vmstat_update
Signed-off-by: Kassey Li
---
Changelog:
v1:
https://lore.kernel.org/all/20240308010929.1955339-1-quic_yinga...@quicinc.com/
v1->v2:
- do not follow checkpatch in TRACE_EVENT() macros
- add sample "workqueue_activate_work: work struct ff80413a78b8
function=vmstat_update"
From a tracing POV,
Reviewed-by: Steven Rostedt (Google)
-- Steve
thank you Steve.
add Tejun to review this change.
---
include/trace/events/workqueue.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/trace/events/workqueue.h b/include/trace/events/workqueue.h
index 262d52021c23..6ef5b7254070 100644
--- a/include/trace/events/workqueue.h
+++ b/include/trace/events/workqueue.h
@@ -64,13 +64,15 @@ TRACE_EVENT(workqueue_activate_work,
TP_STRUCT__entry(
__field( void *,work)
+ __field( void *,function)
),
TP_fast_assign(
__entry->work= work;
+ __entry->function= work->func;
),
- TP_printk("work struct %p", __entry->work)
+ TP_printk("work struct %p function=%ps ", __entry->work,
__entry->function)
);
/**
Re: [RFC][PATCH 0/4] Make bpf_jit and kprobes work with CONFIG_MODULES=n
On Thu, Mar 7, 2024 at 6:50 PM Masami Hiramatsu wrote: > > On Wed, 6 Mar 2024 17:58:14 -0800 > Song Liu wrote: > > > Hi Calvin, > > > > It is great to hear from you! :) > > > > On Wed, Mar 6, 2024 at 3:23 PM Calvin Owens wrote: > > > > > > On Wednesday 03/06 at 13:34 -0800, Luis Chamberlain wrote: > > > > On Wed, Mar 06, 2024 at 12:05:07PM -0800, Calvin Owens wrote: > > > > > Hello all, > > > > > > > > > > This patchset makes it possible to use bpftrace with kprobes on > > > > > kernels > > > > > built without loadable module support. > > > > > > > > This is a step in the right direction for another reason: clearly the > > > > module_alloc() is not about modules, and we have special reasons for it > > > > now beyond modules. The effort to share a generalize a huge page for > > > > these things is also another reason for some of this but that is more > > > > long term. > > > > > > > > I'm all for minor changes here so to avoid regressions but it seems a > > > > rename is in order -- if we're going to all this might as well do it > > > > now. And for that I'd just like to ask you paint the bikeshed with > > > > Song Liu as he's been the one slowly making way to help us get there > > > > with the "module: replace module_layout with module_memory", > > > > and Mike Rapoport as he's had some follow up attempts [0]. As I see it, > > > > the EXECMEM stuff would be what we use instead then. Mike kept the > > > > module_alloc() and the execmem was just a wrapper but your move of the > > > > arch stuff makes sense as well and I think would complement his series > > > > nicely. > > > > > > I apologize for missing that. I think these are the four most recent > > > versions of the different series referenced from that LWN link: > > > > > > a) https://lore.kernel.org/all/20230918072955.2507221-1-r...@kernel.org/ > > > b) https://lore.kernel.org/all/20230526051529.3387103-1-s...@kernel.org/ > > > c) https://lore.kernel.org/all/20221107223921.3451913-1-s...@kernel.org/ > > > d) > > > https://lore.kernel.org/all/20201120202426.18009-1-rick.p.edgeco...@intel.com/ > > > > > > Song and Mike, please correct me if I'm wrong, but I think what I've > > > done here (see [1], sorry for not adding you initially) is compatible > > > with everything both of you have recently proposed above. How do you > > > feel about this as a first step? > > > > I agree that the work here is compatible with other efforts. I have no > > objection to making this the first step. > > > > > > > > For naming, execmem_alloc() seems reasonable to me? I have no strong > > > feelings at all, I'll just use that going forward unless somebody else > > > expresses an opinion. > > > > I am not good at naming things. No objection from me to "execmem_alloc". > > Hm, it sounds good to me too. I think we should add a patch which just > rename the module_alloc/module_memfree with execmem_alloc/free first. I think that would be cleaner, yes. Leaving the possible move to a secondary patch and placing the testing more on the later part. Luis
Re: [RFC][PATCH 0/4] Make bpf_jit and kprobes work with CONFIG_MODULES=n
On Wed, 6 Mar 2024 17:58:14 -0800 Song Liu wrote: > Hi Calvin, > > It is great to hear from you! :) > > On Wed, Mar 6, 2024 at 3:23 PM Calvin Owens wrote: > > > > On Wednesday 03/06 at 13:34 -0800, Luis Chamberlain wrote: > > > On Wed, Mar 06, 2024 at 12:05:07PM -0800, Calvin Owens wrote: > > > > Hello all, > > > > > > > > This patchset makes it possible to use bpftrace with kprobes on kernels > > > > built without loadable module support. > > > > > > This is a step in the right direction for another reason: clearly the > > > module_alloc() is not about modules, and we have special reasons for it > > > now beyond modules. The effort to share a generalize a huge page for > > > these things is also another reason for some of this but that is more > > > long term. > > > > > > I'm all for minor changes here so to avoid regressions but it seems a > > > rename is in order -- if we're going to all this might as well do it > > > now. And for that I'd just like to ask you paint the bikeshed with > > > Song Liu as he's been the one slowly making way to help us get there > > > with the "module: replace module_layout with module_memory", > > > and Mike Rapoport as he's had some follow up attempts [0]. As I see it, > > > the EXECMEM stuff would be what we use instead then. Mike kept the > > > module_alloc() and the execmem was just a wrapper but your move of the > > > arch stuff makes sense as well and I think would complement his series > > > nicely. > > > > I apologize for missing that. I think these are the four most recent > > versions of the different series referenced from that LWN link: > > > > a) https://lore.kernel.org/all/20230918072955.2507221-1-r...@kernel.org/ > > b) https://lore.kernel.org/all/20230526051529.3387103-1-s...@kernel.org/ > > c) https://lore.kernel.org/all/20221107223921.3451913-1-s...@kernel.org/ > > d) > > https://lore.kernel.org/all/20201120202426.18009-1-rick.p.edgeco...@intel.com/ > > > > Song and Mike, please correct me if I'm wrong, but I think what I've > > done here (see [1], sorry for not adding you initially) is compatible > > with everything both of you have recently proposed above. How do you > > feel about this as a first step? > > I agree that the work here is compatible with other efforts. I have no > objection to making this the first step. > > > > > For naming, execmem_alloc() seems reasonable to me? I have no strong > > feelings at all, I'll just use that going forward unless somebody else > > expresses an opinion. > > I am not good at naming things. No objection from me to "execmem_alloc". Hm, it sounds good to me too. I think we should add a patch which just rename the module_alloc/module_memfree with execmem_alloc/free first. Thanks, -- Masami Hiramatsu (Google)
Re: [RFC][PATCH 3/4] kprobes: Allow kprobes with CONFIG_MODULES=n
On Thu, 7 Mar 2024 09:22:07 +0200
Mike Rapoport wrote:
> On Wed, Mar 06, 2024 at 12:05:10PM -0800, Calvin Owens wrote:
> > If something like this is merged down the road, it can go in at leisure
> > once the module_alloc change is in: it's a one-way dependency.
> >
> > Signed-off-by: Calvin Owens
> > ---
> > arch/Kconfig| 2 +-
> > kernel/kprobes.c| 22 ++
> > kernel/trace/trace_kprobe.c | 11 +++
> > 3 files changed, 34 insertions(+), 1 deletion(-)
>
> When I did this in my last execmem posting, I think I've got slightly less
> ugly ifdery, you may want to take a look at that:
>
> https://lore.kernel.org/all/20230918072955.2507221-13-r...@kernel.org
Good catch, and sorry I missed that series last year.
But it seems your patch seems less ugly.
Calvin, can you follow his patch?
Thank you,
>
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index cfc24ced16dd..e60ce984d095 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -52,8 +52,8 @@ config GENERIC_ENTRY
> >
> > config KPROBES
> > bool "Kprobes"
> > - depends on MODULES
> > depends on HAVE_KPROBES
> > + select MODULE_ALLOC
> > select KALLSYMS
> > select TASKS_RCU if PREEMPTION
> > help
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 9d9095e81792..194270e17d57 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -1556,8 +1556,12 @@ static bool is_cfi_preamble_symbol(unsigned long
> > addr)
> > str_has_prefix("__pfx_", symbuf);
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
> > static int check_kprobe_address_safe(struct kprobe *p,
> > struct module **probed_mod)
> > +#else
> > +static int check_kprobe_address_safe(struct kprobe *p)
> > +#endif
> > {
> > int ret;
> >
> > @@ -1580,6 +1584,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> > goto out;
> > }
> >
> > +#if IS_ENABLED(CONFIG_MODULES)
>
> Plain #ifdef will do here and below. IS_ENABLED is for usage withing the
> code, like
>
> if (IS_ENABLED(CONFIG_MODULES))
> ;
>
> > /* Check if 'p' is probing a module. */
> > *probed_mod = __module_text_address((unsigned long) p->addr);
> > if (*probed_mod) {
>
> --
> Sincerely yours,
> Mike.
--
Masami Hiramatsu (Google)
Re: [RFC][PATCH 3/4] kprobes: Allow kprobes with CONFIG_MODULES=n
On Wed, 6 Mar 2024 12:05:10 -0800
Calvin Owens wrote:
> If something like this is merged down the road, it can go in at leisure
> once the module_alloc change is in: it's a one-way dependency.
>
> Signed-off-by: Calvin Owens
> ---
> arch/Kconfig| 2 +-
> kernel/kprobes.c| 22 ++
> kernel/trace/trace_kprobe.c | 11 +++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index cfc24ced16dd..e60ce984d095 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,8 +52,8 @@ config GENERIC_ENTRY
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> depends on HAVE_KPROBES
> + select MODULE_ALLOC
OK, if we use EXEC_ALLOC,
config EXEC_ALLOC
depends on HAVE_EXEC_ALLOC
And
config KPROBES
bool "Kprobes"
depends on MODULES || EXEC_ALLOC
select EXEC_ALLOC if HAVE_EXEC_ALLOC
then kprobes can be enabled either modules supported or exec_alloc is supported.
(new arch does not need to implement exec_alloc)
Maybe we also need something like
#ifdef CONFIG_EXEC_ALLOC
#define module_alloc(size) exec_alloc(size)
#endif
in kprobes.h, or just add `replacing module_alloc with exec_alloc` patch.
Thank you,
> select KALLSYMS
> select TASKS_RCU if PREEMPTION
> help
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 9d9095e81792..194270e17d57 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1556,8 +1556,12 @@ static bool is_cfi_preamble_symbol(unsigned long addr)
> str_has_prefix("__pfx_", symbuf);
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> static int check_kprobe_address_safe(struct kprobe *p,
>struct module **probed_mod)
> +#else
> +static int check_kprobe_address_safe(struct kprobe *p)
> +#endif
> {
> int ret;
>
> @@ -1580,6 +1584,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> goto out;
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> /* Check if 'p' is probing a module. */
> *probed_mod = __module_text_address((unsigned long) p->addr);
> if (*probed_mod) {
> @@ -1603,6 +1608,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> ret = -ENOENT;
> }
> }
> +#endif
> +
> out:
> preempt_enable();
> jump_label_unlock();
> @@ -1614,7 +1621,9 @@ int register_kprobe(struct kprobe *p)
> {
> int ret;
> struct kprobe *old_p;
> +#if IS_ENABLED(CONFIG_MODULES)
> struct module *probed_mod;
> +#endif
> kprobe_opcode_t *addr;
> bool on_func_entry;
>
> @@ -1633,7 +1642,11 @@ int register_kprobe(struct kprobe *p)
> p->nmissed = 0;
> INIT_LIST_HEAD(>list);
>
> +#if IS_ENABLED(CONFIG_MODULES)
> ret = check_kprobe_address_safe(p, _mod);
> +#else
> + ret = check_kprobe_address_safe(p);
> +#endif
> if (ret)
> return ret;
>
> @@ -1676,8 +1689,10 @@ int register_kprobe(struct kprobe *p)
> out:
> mutex_unlock(_mutex);
>
> +#if IS_ENABLED(CONFIG_MODULES)
> if (probed_mod)
> module_put(probed_mod);
> +#endif
>
> return ret;
> }
> @@ -2482,6 +2497,7 @@ int kprobe_add_area_blacklist(unsigned long start,
> unsigned long end)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> /* Remove all symbols in given area from kprobe blacklist */
> static void kprobe_remove_area_blacklist(unsigned long start, unsigned long
> end)
> {
> @@ -2499,6 +2515,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long
> entry)
> {
> kprobe_remove_area_blacklist(entry, entry + 1);
> }
> +#endif
>
> int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long
> *value,
> char *type, char *sym)
> @@ -2564,6 +2581,7 @@ static int __init populate_kprobe_blacklist(unsigned
> long *start,
> return ret ? : arch_populate_kprobe_blacklist();
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> static void add_module_kprobe_blacklist(struct module *mod)
> {
> unsigned long start, end;
> @@ -2665,6 +2683,7 @@ static struct notifier_block kprobe_module_nb = {
> .notifier_call = kprobes_module_callback,
> .priority = 0
> };
> +#endif /* IS_ENABLED(CONFIG_MODULES) */
>
> void kprobe_free_init_mem(void)
> {
> @@ -2724,8 +2743,11 @@ static int __init init_kprobes(void)
> err = arch_init_kprobes();
> if (!err)
> err = register_die_notifier(_exceptions_nb);
> +
> +#if IS_ENABLED(CONFIG_MODULES)
> if (!err)
> err = register_module_notifier(_module_nb);
> +#endif
>
> kprobes_initialized = (err == 0);
> kprobe_sysctls_init();
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index c4c6e0e0068b..dd4598f775b9 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -102,6 +102,7 @@ static
Re: [RFC][PATCH 0/4] Make bpf_jit and kprobes work with CONFIG_MODULES=n
Hi, On Wed, 6 Mar 2024 13:34:40 -0800 Luis Chamberlain wrote: > On Wed, Mar 06, 2024 at 12:05:07PM -0800, Calvin Owens wrote: > > Hello all, > > > > This patchset makes it possible to use bpftrace with kprobes on kernels > > built without loadable module support. > > This is a step in the right direction for another reason: clearly the > module_alloc() is not about modules, and we have special reasons for it > now beyond modules. The effort to share a generalize a huge page for > these things is also another reason for some of this but that is more > long term. Indeed. If it works without CONFIG_MODULES, it may be exec_alloc() or something like that. Anyway, thanks for great job on this item! > > I'm all for minor changes here so to avoid regressions but it seems a > rename is in order -- if we're going to all this might as well do it > now. And for that I'd just like to ask you paint the bikeshed with > Song Liu as he's been the one slowly making way to help us get there > with the "module: replace module_layout with module_memory", > and Mike Rapoport as he's had some follow up attempts [0]. As I see it, > the EXECMEM stuff would be what we use instead then. Mike kept the > module_alloc() and the execmem was just a wrapper but your move of the > arch stuff makes sense as well and I think would complement his series > nicely. yeah, it is better to work with Mike. Thank you, > > If you're gonna split code up to move to another place, it'd be nice > if you can add copyright headers as was done with the kernel/module.c > split into kernel/module/*.c > > Can we start with some small basic stuff we can all agree on? > > [0] https://lwn.net/Articles/944857/ > > Luis -- Masami Hiramatsu (Google)
Re: [PATCH v2] workqueue: add function in event of workqueue_activate_work
On Fri, 8 Mar 2024 10:18:18 +0800
Kassey Li wrote:
> The trace event "workqueue_activate_work" only print work struct.
> However, function is the region of interest in a full sequence of work.
> Current workqueue_activate_work trace event output:
>
> workqueue_activate_work: work struct ff88b4a0f450
>
> With this change, workqueue_activate_work will print the function name,
> align with workqueue_queue_work/execute_start/execute_end event.
>
> workqueue_activate_work: work struct ff80413a78b8
> function=vmstat_update
>
> Signed-off-by: Kassey Li
> ---
> Changelog:
> v1:
> https://lore.kernel.org/all/20240308010929.1955339-1-quic_yinga...@quicinc.com/
> v1->v2:
> - do not follow checkpatch in TRACE_EVENT() macros
> - add sample "workqueue_activate_work: work struct ff80413a78b8
> function=vmstat_update"
From a tracing POV,
Reviewed-by: Steven Rostedt (Google)
-- Steve
> ---
> include/trace/events/workqueue.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/trace/events/workqueue.h
> b/include/trace/events/workqueue.h
> index 262d52021c23..6ef5b7254070 100644
> --- a/include/trace/events/workqueue.h
> +++ b/include/trace/events/workqueue.h
> @@ -64,13 +64,15 @@ TRACE_EVENT(workqueue_activate_work,
>
> TP_STRUCT__entry(
> __field( void *,work)
> + __field( void *,function)
> ),
>
> TP_fast_assign(
> __entry->work = work;
> + __entry->function = work->func;
> ),
>
> - TP_printk("work struct %p", __entry->work)
> + TP_printk("work struct %p function=%ps ", __entry->work,
> __entry->function)
> );
>
> /**
Re: [PATCH] workqueue: add function in event of workqueue_activate_work
On 2024/3/8 9:50, Steven Rostedt wrote:
On Fri, 8 Mar 2024 09:09:29 +0800
Kassey Li wrote:
The trace event "workqueue_activate_work" only print work struct.
However, function is the region of interest in a full sequence of work.
Current workqueue_activate_work trace event output:
workqueue_activate_work: work struct ff88b4a0f450
With this change, workqueue_activate_work will print the function name,
align with workqueue_queue_work/execute_start/execute_end event.
checkpatch.pl will report below error for the space:
ERROR: space prohibited after that open parenthesis '('
#28: FILE: include/trace/events/workqueue.h:67:
+ __field( void *,function)
total: 1 errors, 0 warnings, 16 lines checked
fix this error.
Signed-off-by: Kassey Li
---
include/trace/events/workqueue.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/workqueue.h b/include/trace/events/workqueue.h
index 262d52021c23..a42c1a293459 100644
--- a/include/trace/events/workqueue.h
+++ b/include/trace/events/workqueue.h
@@ -63,14 +63,16 @@ TRACE_EVENT(workqueue_activate_work,
TP_ARGS(work),
TP_STRUCT__entry(
- __field( void *,work)
+ __field(void *, work)
+ __field(void *, function)
Note, please do not follow checkpatch in TRACE_EVENT() macros. It simply
doesn't understand it. The above is supposed to be similar to structure
formatting.
ie:
struct __entry {
void*work;
void*function;
};
TP_STRUCT__entry(
__field( void *,work)
__field( void *,function)
),
That looks much better.
-- Steve
thanks for the remind.
send out the v2 as your suggest:
https://lore.kernel.org/all/20240308021818.2306176-1-quic_yinga...@quicinc.com/
TP_fast_assign(
__entry->work= work;
+ __entry->function= work->func;
),
- TP_printk("work struct %p", __entry->work)
+ TP_printk("work struct %p function=%ps ", __entry->work,
__entry->function)
);
/**
[PATCH v2] workqueue: add function in event of workqueue_activate_work
The trace event "workqueue_activate_work" only print work struct.
However, function is the region of interest in a full sequence of work.
Current workqueue_activate_work trace event output:
workqueue_activate_work: work struct ff88b4a0f450
With this change, workqueue_activate_work will print the function name,
align with workqueue_queue_work/execute_start/execute_end event.
workqueue_activate_work: work struct ff80413a78b8 function=vmstat_update
Signed-off-by: Kassey Li
---
Changelog:
v1:
https://lore.kernel.org/all/20240308010929.1955339-1-quic_yinga...@quicinc.com/
v1->v2:
- do not follow checkpatch in TRACE_EVENT() macros
- add sample "workqueue_activate_work: work struct ff80413a78b8
function=vmstat_update"
---
include/trace/events/workqueue.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/trace/events/workqueue.h b/include/trace/events/workqueue.h
index 262d52021c23..6ef5b7254070 100644
--- a/include/trace/events/workqueue.h
+++ b/include/trace/events/workqueue.h
@@ -64,13 +64,15 @@ TRACE_EVENT(workqueue_activate_work,
TP_STRUCT__entry(
__field( void *,work)
+ __field( void *,function)
),
TP_fast_assign(
__entry->work = work;
+ __entry->function = work->func;
),
- TP_printk("work struct %p", __entry->work)
+ TP_printk("work struct %p function=%ps ", __entry->work,
__entry->function)
);
/**
--
2.25.1
Re: [RFC][PATCH 1/4] module: mm: Make module_alloc() generally available
Hi Calvin, On Wed, 6 Mar 2024 12:05:08 -0800 Calvin Owens wrote: > Both BPF_JIT and KPROBES depend on CONFIG_MODULES, but only require > module_alloc() itself, which can be easily separated into a standalone > allocator for executable kernel memory. Thanks for your work! As Luis pointed, it is better to use different name because this is not only for modules and it does not depend on CONFIG_MODULES. > > Thomas Gleixner sent a patch to do that for x86 as part of a larger > series a couple years ago: > > https://lore.kernel.org/all/20220716230953.442937...@linutronix.de/ > > I've simply extended that approach to the whole kernel. I would like to see a series of patches for each architecture so that architecture maintainers carefully check and test this feature. What about introducing CONFIG_HAVE_EXEC_ALLOC and enable it on each architecture? Then you can start small set of major architectures and expand it later. Thank you, > > Signed-off-by: Calvin Owens > --- > arch/Kconfig | 2 +- > arch/arm/kernel/module.c | 35 - > arch/arm/mm/Makefile | 2 + > arch/arm/mm/module_alloc.c | 40 ++ > arch/arm64/kernel/module.c | 127 -- > arch/arm64/mm/Makefile | 1 + > arch/arm64/mm/module_alloc.c | 130 +++ > arch/loongarch/kernel/module.c | 6 -- > arch/loongarch/mm/Makefile | 2 + > arch/loongarch/mm/module_alloc.c | 10 +++ > arch/mips/kernel/module.c| 10 --- > arch/mips/mm/Makefile| 2 + > arch/mips/mm/module_alloc.c | 13 > arch/nios2/kernel/module.c | 20 - > arch/nios2/mm/Makefile | 2 + > arch/nios2/mm/module_alloc.c | 22 ++ > arch/parisc/kernel/module.c | 12 --- > arch/parisc/mm/Makefile | 1 + > arch/parisc/mm/module_alloc.c| 15 > arch/powerpc/kernel/module.c | 36 - > arch/powerpc/mm/Makefile | 1 + > arch/powerpc/mm/module_alloc.c | 41 ++ > arch/riscv/kernel/module.c | 11 --- > arch/riscv/mm/Makefile | 1 + > arch/riscv/mm/module_alloc.c | 17 > arch/s390/kernel/module.c| 37 - > arch/s390/mm/Makefile| 1 + > arch/s390/mm/module_alloc.c | 42 ++ > arch/sparc/kernel/module.c | 31 > arch/sparc/mm/Makefile | 2 + > arch/sparc/mm/module_alloc.c | 31 > arch/x86/kernel/ftrace.c | 2 +- > arch/x86/kernel/module.c | 56 - > arch/x86/mm/Makefile | 2 + > arch/x86/mm/module_alloc.c | 59 ++ > fs/proc/kcore.c | 2 +- > kernel/module/Kconfig| 1 + > kernel/module/main.c | 17 > mm/Kconfig | 3 + > mm/Makefile | 1 + > mm/module_alloc.c| 21 + > mm/vmalloc.c | 2 +- > 42 files changed, 467 insertions(+), 402 deletions(-) > create mode 100644 arch/arm/mm/module_alloc.c > create mode 100644 arch/arm64/mm/module_alloc.c > create mode 100644 arch/loongarch/mm/module_alloc.c > create mode 100644 arch/mips/mm/module_alloc.c > create mode 100644 arch/nios2/mm/module_alloc.c > create mode 100644 arch/parisc/mm/module_alloc.c > create mode 100644 arch/powerpc/mm/module_alloc.c > create mode 100644 arch/riscv/mm/module_alloc.c > create mode 100644 arch/s390/mm/module_alloc.c > create mode 100644 arch/sparc/mm/module_alloc.c > create mode 100644 arch/x86/mm/module_alloc.c > create mode 100644 mm/module_alloc.c > > diff --git a/arch/Kconfig b/arch/Kconfig > index a5af0edd3eb8..cfc24ced16dd 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -1305,7 +1305,7 @@ config ARCH_HAS_STRICT_MODULE_RWX > > config STRICT_MODULE_RWX > bool "Set loadable kernel module data as NX and text as RO" if > ARCH_OPTIONAL_KERNEL_RWX > - depends on ARCH_HAS_STRICT_MODULE_RWX && MODULES > + depends on ARCH_HAS_STRICT_MODULE_RWX && MODULE_ALLOC > default !ARCH_OPTIONAL_KERNEL_RWX || ARCH_OPTIONAL_KERNEL_RWX_DEFAULT > help > If this is set, module text and rodata memory will be made read-only, > diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c > index e74d84f58b77..1c8798732d12 100644 > --- a/arch/arm/kernel/module.c > +++ b/arch/arm/kernel/module.c > @@ -4,15 +4,12 @@ > * > * Copyright (C) 2002 Russell King. > * Modified for nommu by Hyok S. Choi > - * > - * Module allocation method suggested by Andi Kleen. > */ > #include > #include > #include > #include > #include > -#include > #include > #include > #include > @@ -22,38 +19,6 @@ > #include > #include > > -#ifdef CONFIG_XIP_KERNEL > -/* > - * The XIP kernel text is mapped in the module area for modules and > - * some other stuff to work without any indirect relocations. > - *
Re: [PATCH] workqueue: add function in event of workqueue_activate_work
On Fri, 8 Mar 2024 09:09:29 +0800
Kassey Li wrote:
> The trace event "workqueue_activate_work" only print work struct.
> However, function is the region of interest in a full sequence of work.
> Current workqueue_activate_work trace event output:
>
> workqueue_activate_work: work struct ff88b4a0f450
>
> With this change, workqueue_activate_work will print the function name,
> align with workqueue_queue_work/execute_start/execute_end event.
>
> checkpatch.pl will report below error for the space:
>
> ERROR: space prohibited after that open parenthesis '('
> #28: FILE: include/trace/events/workqueue.h:67:
> + __field( void *,function)
>
> total: 1 errors, 0 warnings, 16 lines checked
>
> fix this error.
>
> Signed-off-by: Kassey Li
> ---
> include/trace/events/workqueue.h | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/trace/events/workqueue.h
> b/include/trace/events/workqueue.h
> index 262d52021c23..a42c1a293459 100644
> --- a/include/trace/events/workqueue.h
> +++ b/include/trace/events/workqueue.h
> @@ -63,14 +63,16 @@ TRACE_EVENT(workqueue_activate_work,
> TP_ARGS(work),
>
> TP_STRUCT__entry(
> - __field( void *,work)
> + __field(void *, work)
> + __field(void *, function)
Note, please do not follow checkpatch in TRACE_EVENT() macros. It simply
doesn't understand it. The above is supposed to be similar to structure
formatting.
ie:
struct __entry {
void*work;
void*function;
};
TP_STRUCT__entry(
__field( void *,work)
__field( void *,function)
),
That looks much better.
-- Steve
>
> TP_fast_assign(
> __entry->work = work;
> + __entry->function = work->func;
> ),
>
> - TP_printk("work struct %p", __entry->work)
> + TP_printk("work struct %p function=%ps ", __entry->work,
> __entry->function)
> );
>
> /**
[PATCH] workqueue: add function in event of workqueue_activate_work
The trace event "workqueue_activate_work" only print work struct.
However, function is the region of interest in a full sequence of work.
Current workqueue_activate_work trace event output:
workqueue_activate_work: work struct ff88b4a0f450
With this change, workqueue_activate_work will print the function name,
align with workqueue_queue_work/execute_start/execute_end event.
checkpatch.pl will report below error for the space:
ERROR: space prohibited after that open parenthesis '('
#28: FILE: include/trace/events/workqueue.h:67:
+ __field( void *,function)
total: 1 errors, 0 warnings, 16 lines checked
fix this error.
Signed-off-by: Kassey Li
---
include/trace/events/workqueue.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/workqueue.h b/include/trace/events/workqueue.h
index 262d52021c23..a42c1a293459 100644
--- a/include/trace/events/workqueue.h
+++ b/include/trace/events/workqueue.h
@@ -63,14 +63,16 @@ TRACE_EVENT(workqueue_activate_work,
TP_ARGS(work),
TP_STRUCT__entry(
- __field( void *,work)
+ __field(void *, work)
+ __field(void *, function)
),
TP_fast_assign(
__entry->work = work;
+ __entry->function = work->func;
),
- TP_printk("work struct %p", __entry->work)
+ TP_printk("work struct %p function=%ps ", __entry->work,
__entry->function)
);
/**
--
2.25.1
Re: [RFC][PATCH 3/4] kprobes: Allow kprobes with CONFIG_MODULES=n
Le 06/03/2024 à 21:05, Calvin Owens a écrit :
> [Vous ne recevez pas souvent de courriers de jcalvinow...@gmail.com.
> Découvrez pourquoi ceci est important à
> https://aka.ms/LearnAboutSenderIdentification ]
>
> If something like this is merged down the road, it can go in at leisure
> once the module_alloc change is in: it's a one-way dependency.
Too many #ifdef, please reorganise stuff to avoid that and avoid
changing prototypes based of CONFIG_MODULES.
Other few comments below.
>
> Signed-off-by: Calvin Owens
> ---
> arch/Kconfig| 2 +-
> kernel/kprobes.c| 22 ++
> kernel/trace/trace_kprobe.c | 11 +++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index cfc24ced16dd..e60ce984d095 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -52,8 +52,8 @@ config GENERIC_ENTRY
>
> config KPROBES
> bool "Kprobes"
> - depends on MODULES
> depends on HAVE_KPROBES
> + select MODULE_ALLOC
> select KALLSYMS
> select TASKS_RCU if PREEMPTION
> help
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 9d9095e81792..194270e17d57 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1556,8 +1556,12 @@ static bool is_cfi_preamble_symbol(unsigned long addr)
> str_has_prefix("__pfx_", symbuf);
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> static int check_kprobe_address_safe(struct kprobe *p,
> struct module **probed_mod)
> +#else
> +static int check_kprobe_address_safe(struct kprobe *p)
> +#endif
A bit ugly to have to change the prototype, why not just keep probed_mod
at all time ?
When CONFIG_MODULES is not selected, __module_text_address() returns
NULL so it should work without that many #ifdefs.
> {
> int ret;
>
> @@ -1580,6 +1584,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> goto out;
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> /* Check if 'p' is probing a module. */
> *probed_mod = __module_text_address((unsigned long) p->addr);
> if (*probed_mod) {
> @@ -1603,6 +1608,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
> ret = -ENOENT;
> }
> }
> +#endif
> +
> out:
> preempt_enable();
> jump_label_unlock();
> @@ -1614,7 +1621,9 @@ int register_kprobe(struct kprobe *p)
> {
> int ret;
> struct kprobe *old_p;
> +#if IS_ENABLED(CONFIG_MODULES)
> struct module *probed_mod;
> +#endif
> kprobe_opcode_t *addr;
> bool on_func_entry;
>
> @@ -1633,7 +1642,11 @@ int register_kprobe(struct kprobe *p)
> p->nmissed = 0;
> INIT_LIST_HEAD(>list);
>
> +#if IS_ENABLED(CONFIG_MODULES)
> ret = check_kprobe_address_safe(p, _mod);
> +#else
> + ret = check_kprobe_address_safe(p);
> +#endif
> if (ret)
> return ret;
>
> @@ -1676,8 +1689,10 @@ int register_kprobe(struct kprobe *p)
> out:
> mutex_unlock(_mutex);
>
> +#if IS_ENABLED(CONFIG_MODULES)
> if (probed_mod)
> module_put(probed_mod);
> +#endif
>
> return ret;
> }
> @@ -2482,6 +2497,7 @@ int kprobe_add_area_blacklist(unsigned long start,
> unsigned long end)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> /* Remove all symbols in given area from kprobe blacklist */
> static void kprobe_remove_area_blacklist(unsigned long start, unsigned long
> end)
> {
> @@ -2499,6 +2515,7 @@ static void kprobe_remove_ksym_blacklist(unsigned long
> entry)
> {
> kprobe_remove_area_blacklist(entry, entry + 1);
> }
> +#endif
>
> int __weak arch_kprobe_get_kallsym(unsigned int *symnum, unsigned long
> *value,
> char *type, char *sym)
> @@ -2564,6 +2581,7 @@ static int __init populate_kprobe_blacklist(unsigned
> long *start,
> return ret ? : arch_populate_kprobe_blacklist();
> }
>
> +#if IS_ENABLED(CONFIG_MODULES)
> static void add_module_kprobe_blacklist(struct module *mod)
> {
> unsigned long start, end;
> @@ -2665,6 +2683,7 @@ static struct notifier_block kprobe_module_nb = {
> .notifier_call = kprobes_module_callback,
> .priority = 0
> };
> +#endif /* IS_ENABLED(CONFIG_MODULES) */
>
> void kprobe_free_init_mem(void)
> {
> @@ -2724,8 +2743,11 @@ static int __init init_kprobes(void)
> err = arch_init_kprobes();
> if (!err)
> err = register_die_notifier(_exceptions_nb);
> +
> +#if IS_ENABLED(CONFIG_MODULES)
> if (!err)
> err = register_module_notifier(_module_nb);
> +#endif
>
> kprobes_initialized = (err == 0);
> kprobe_sysctls_init();
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index
Re: [RFC][PATCH 2/4] bpf: Allow BPF_JIT with CONFIG_MODULES=n
Le 06/03/2024 à 21:05, Calvin Owens a écrit :
> [Vous ne recevez pas souvent de courriers de jcalvinow...@gmail.com.
> Découvrez pourquoi ceci est important à
> https://aka.ms/LearnAboutSenderIdentification ]
>
> No BPF code has to change, except in struct_ops (for module refs).
>
> This conflicts with bpf-next because of this (relevant) series:
>
> https://lore.kernel.org/all/20240119225005.668602-1-thinker...@gmail.com/
>
> If something like this is merged down the road, it can go through
> bpf-next at leisure once the module_alloc change is in: it's a one-way
> dependency.
>
> Signed-off-by: Calvin Owens
> ---
> kernel/bpf/Kconfig | 2 +-
> kernel/bpf/bpf_struct_ops.c | 28
> 2 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
> index 6a906ff93006..77df483a8925 100644
> --- a/kernel/bpf/Kconfig
> +++ b/kernel/bpf/Kconfig
> @@ -42,7 +42,7 @@ config BPF_JIT
> bool "Enable BPF Just In Time compiler"
> depends on BPF
> depends on HAVE_CBPF_JIT || HAVE_EBPF_JIT
> - depends on MODULES
> + select MODULE_ALLOC
> help
>BPF programs are normally handled by a BPF interpreter. This option
>allows the kernel to generate native code when a program is loaded
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 02068bd0e4d9..fbf08a1bb00c 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -108,11 +108,30 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
> #endif
> };
>
> +#if IS_ENABLED(CONFIG_MODULES)
Can you avoid ifdefs as much as possible ?
> static const struct btf_type *module_type;
>
> +static int bpf_struct_module_type_init(struct btf *btf)
> +{
> + s32 module_id;
Could be:
if (!IS_ENABLED(CONFIG_MODULES))
return 0;
> +
> + module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> + if (module_id < 0)
> + return 1;
> +
> + module_type = btf_type_by_id(btf, module_id);
> + return 0;
> +}
> +#else
> +static int bpf_struct_module_type_init(struct btf *btf)
> +{
> + return 0;
> +}
> +#endif
> +
> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> {
> - s32 type_id, value_id, module_id;
> + s32 type_id, value_id;
> const struct btf_member *member;
> struct bpf_struct_ops *st_ops;
> const struct btf_type *t;
> @@ -125,12 +144,10 @@ void bpf_struct_ops_init(struct btf *btf, struct
> bpf_verifier_log *log)
> #include "bpf_struct_ops_types.h"
> #undef BPF_STRUCT_OPS_TYPE
>
> - module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> - if (module_id < 0) {
> + if (bpf_struct_module_type_init(btf)) {
> pr_warn("Cannot find struct module in btf_vmlinux\n");
> return;
> }
> - module_type = btf_type_by_id(btf, module_id);
>
> for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> st_ops = bpf_struct_ops[i];
> @@ -433,12 +450,15 @@ static long bpf_struct_ops_map_update_elem(struct
> bpf_map *map, void *key,
>
> moff = __btf_member_bit_offset(t, member) / 8;
> ptype = btf_type_resolve_ptr(btf_vmlinux, member->type,
> NULL);
> +
> +#if IS_ENABLED(CONFIG_MODULES)
Can't see anything depending on CONFIG_MODULES here, can you instead do:
if (IS_ENABLED(CONFIG_MODULES) && ptype == module_type) {
> if (ptype == module_type) {
> if (*(void **)(udata + moff))
> goto reset_unlock;
> *(void **)(kdata + moff) = BPF_MODULE_OWNER;
> continue;
> }
> +#endif
>
> err = st_ops->init_member(t, member, kdata, udata);
> if (err < 0)
> --
> 2.43.0
>
>
[PATCH] ipvs: allow netlink configuration from non-initial user namespace
Configuring ipvs in a non-initial user namespace using the genl
netlink interface, e.g., by 'ipvsadm' is currently resulting in an
'-EPERM'. This is due to the use of GENL_ADMIN_PERM flag in
'ip_vs_ctl.c'.
Similarly to other genl interfaces, we switch to the use of
GENL_UNS_ADMIN_PERM flag which allows connection from non-initial
user namespace. Thus, it would be feasible to configure ipvs using
the genl interface also from within an unprivileged system container.
Since adding of new services and new dests are triggered from
userspace, accounting for the corresponding memory allocations in
ip_vs_new_dest() and ip_vs_add_service() is activated.
We tested this by simply running some samples from "man ipvsadm"
within an unprivileged user namespaced system container in GyroidOS.
Further, we successfully passed an adapted version of the ipvs
selftest in 'tools/testing/selftests/netfilter/ipvs.sh' using
preliminary created network namespaces from unprivileged GyroidOS
containers.
Signed-off-by: Michael Weiß
---
net/netfilter/ipvs/ip_vs_ctl.c | 36 +-
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 143a341bbc0a..d39120c64207 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -1080,7 +1080,7 @@ ip_vs_new_dest(struct ip_vs_service *svc, struct
ip_vs_dest_user_kern *udest)
return -EINVAL;
}
- dest = kzalloc(sizeof(struct ip_vs_dest), GFP_KERNEL);
+ dest = kzalloc(sizeof(struct ip_vs_dest), GFP_KERNEL_ACCOUNT);
if (dest == NULL)
return -ENOMEM;
@@ -1421,7 +1421,7 @@ ip_vs_add_service(struct netns_ipvs *ipvs, struct
ip_vs_service_user_kern *u,
ret_hooks = ret;
}
- svc = kzalloc(sizeof(struct ip_vs_service), GFP_KERNEL);
+ svc = kzalloc(sizeof(struct ip_vs_service), GFP_KERNEL_ACCOUNT);
if (svc == NULL) {
IP_VS_DBG(1, "%s(): no memory\n", __func__);
ret = -ENOMEM;
@@ -4139,98 +4139,98 @@ static const struct genl_small_ops ip_vs_genl_ops[] = {
{
.cmd= IPVS_CMD_NEW_SERVICE,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_SET_SERVICE,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_DEL_SERVICE,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_GET_SERVICE,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_get_cmd,
.dumpit = ip_vs_genl_dump_services,
},
{
.cmd= IPVS_CMD_NEW_DEST,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_SET_DEST,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_DEL_DEST,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_cmd,
},
{
.cmd= IPVS_CMD_GET_DEST,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.dumpit = ip_vs_genl_dump_dests,
},
{
.cmd= IPVS_CMD_NEW_DAEMON,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags = GENL_UNS_ADMIN_PERM,
.doit = ip_vs_genl_set_daemon,
},
{
.cmd= IPVS_CMD_DEL_DAEMON,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
- .flags = GENL_ADMIN_PERM,
+ .flags =
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
Hi Björn, On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel wrote: > > Puranjay! > > Puranjay Mohan writes: > > > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V. > > This allows each ftrace callsite to provide an ftrace_ops to the common > > ftrace trampoline, allowing each callsite to invoke distinct tracer > > functions without the need to fall back to list processing or to > > allocate custom trampolines for each callsite. This significantly speeds > > up cases where multiple distinct trace functions are used and callsites > > are mostly traced by a single tracer. > > > > The idea and most of the implementation is taken from the ARM64's > > implementation of the same feature. The idea is to place a pointer to > > the ftrace_ops as a literal at a fixed offset from the function entry > > point, which can be recovered by the common ftrace trampoline. > > Not really a review, but some more background; Another rationale (on-top > of the improved per-call performance!) for CALL_OPS was to use it to > build ftrace direct call support (which BPF uses a lot!). Mark, please > correct me if I'm lying here! > > On Arm64, CALL_OPS makes it possible to implement direct calls, while > only patching one BL instruction -- nice! > > On RISC-V we cannot use use the same ideas as Arm64 straight off, > because the range of jal (compare to BL) is simply too short (+/-1M). > So, on RISC-V we need to use a full auipc/jal pair (the text patching > story is another chapter, but let's leave that aside for now). Since we > have to patch multiple instructions, the cmodx situation doesn't really > improve with CALL_OPS. > > Let's say that we continue building on your patch and implement direct > calls on CALL_OPS for RISC-V as well. > > From Florent's commit message for direct calls: > > |There are a few cases to distinguish: > |- If a direct call ops is the only one tracing a function: > | - If the direct called trampoline is within the reach of a BL > |instruction > | -> the ftrace patchsite jumps to the trampoline > | - Else > | -> the ftrace patchsite jumps to the ftrace_caller trampoline > which > |reads the ops pointer in the patchsite and jumps to the direct > |call address stored in the ops > |- Else > | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its > | ops literal points to ftrace_list_ops so it iterates over all > | registered ftrace ops, including the direct call ops and calls its > | call_direct_funcs handler which stores the direct called > | trampoline's address in the ftrace_regs and the ftrace_caller > | trampoline will return to that address instead of returning to the > | traced function > > On RISC-V, where auipc/jalr is used, the direct called trampoline would > always be reachable, and then first Else-clause would never be entered. > This means the the performance for direct calls would be the same as the > one we have today (i.e. no regression!). > > RISC-V does like x86 does (-ish) -- patch multiple instructions, long > reach. > > Arm64 uses CALL_OPS and patch one instruction BL. > > Now, with this background in mind, compared to what we have today, > CALL_OPS would give us (again assuming we're using it for direct calls): > > * Better performance for tracer per-call (faster ops lookup) GOOD ^ this was the only motivation for me to implement this patch. I don't think implementing direct calls over call ops is fruitful for RISC-V because once the auipc/jalr can be patched atomically, the direct call trampoline is always reachable. Solving the atomic text patching problem would be fun!! I am eager to see how it will be solved. > * Larger text size (function alignment + extra nops) BAD > * Same direct call performance NEUTRAL > * Same complicated text patching required NEUTRAL > > It would be interesting to see how the per-call performance would > improve on x86 with CALL_OPS! ;-) If I remember from Steven's talk, x86 uses dynamically allocated trampolines for per callsite tracers, would CALL_OPS provide better performance than that? > > I'm trying to wrap my head if it makes sense to have it on RISC-V, given > that we're a bit different from Arm64. Does the scale tip to the GOOD > side? > > Oh, and we really need to see performance numbers on real HW! I have a > VF2 that I could try this series on. It would be great if you can do it :D. Thanks, Puranjay
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu,
> > + do {
> > + state_reg = readl(priv->reset_cfg.state_reg);
> > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > +
> > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > + *rst_ack);
> > + return -EINVAL;
> > + }
> > +
> > + /* expected ACK value read */
> > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
>
> I'm not sure why the second condition in this if() statement is needed. As
> far
> as I can tell the first condition will trigger and the second one won't be
> reached.
The second condition takes care of the following: exp_ack and *rst_ack are
both 0.
This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
we expect the RST_ACK to be 00 afterwards.
> > +/**
> > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > + * @rproc: pointer to the remote processor object
> > + * @fw: pointer to the firmware
> > + *
> > + * Does nothing currently.
> > + *
> > + * Return:
> > + *
> > + * 0 for success.
> > + */
> > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > +{
>
> What is the point of doing rproc_of_parse_firmware() if the firmware image is
> not loaded to memory? Does the remote processor have some kind of default ROM
> image to run if it doesn't find anything in memory?
Yes, the remote processor has a default FW image already loaded by default.
rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the
filesystem or a filename
provided.
Please correct me if I'm wrong.
[1]:
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
[2]:
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
> > +module_platform_driver(arm_rproc_driver);
> > +
>
> I am echoing Krzysztof view about how generic this driver name is. This has
> to
> be related to a family of processors or be made less generic in some way.
> Have
> a look at what TI did for their K3 lineup [1] - I would like to see the same
> thing done here.
Thank you, I'll take care of that and of all the other comments made.
Cheers,
Abdellatif
Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
Puranjay! Puranjay Mohan writes: > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V. > This allows each ftrace callsite to provide an ftrace_ops to the common > ftrace trampoline, allowing each callsite to invoke distinct tracer > functions without the need to fall back to list processing or to > allocate custom trampolines for each callsite. This significantly speeds > up cases where multiple distinct trace functions are used and callsites > are mostly traced by a single tracer. > > The idea and most of the implementation is taken from the ARM64's > implementation of the same feature. The idea is to place a pointer to > the ftrace_ops as a literal at a fixed offset from the function entry > point, which can be recovered by the common ftrace trampoline. Not really a review, but some more background; Another rationale (on-top of the improved per-call performance!) for CALL_OPS was to use it to build ftrace direct call support (which BPF uses a lot!). Mark, please correct me if I'm lying here! On Arm64, CALL_OPS makes it possible to implement direct calls, while only patching one BL instruction -- nice! On RISC-V we cannot use use the same ideas as Arm64 straight off, because the range of jal (compare to BL) is simply too short (+/-1M). So, on RISC-V we need to use a full auipc/jal pair (the text patching story is another chapter, but let's leave that aside for now). Since we have to patch multiple instructions, the cmodx situation doesn't really improve with CALL_OPS. Let's say that we continue building on your patch and implement direct calls on CALL_OPS for RISC-V as well. >From Florent's commit message for direct calls: |There are a few cases to distinguish: |- If a direct call ops is the only one tracing a function: | - If the direct called trampoline is within the reach of a BL |instruction | -> the ftrace patchsite jumps to the trampoline | - Else | -> the ftrace patchsite jumps to the ftrace_caller trampoline which |reads the ops pointer in the patchsite and jumps to the direct |call address stored in the ops |- Else | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its | ops literal points to ftrace_list_ops so it iterates over all | registered ftrace ops, including the direct call ops and calls its | call_direct_funcs handler which stores the direct called | trampoline's address in the ftrace_regs and the ftrace_caller | trampoline will return to that address instead of returning to the | traced function On RISC-V, where auipc/jalr is used, the direct called trampoline would always be reachable, and then first Else-clause would never be entered. This means the the performance for direct calls would be the same as the one we have today (i.e. no regression!). RISC-V does like x86 does (-ish) -- patch multiple instructions, long reach. Arm64 uses CALL_OPS and patch one instruction BL. Now, with this background in mind, compared to what we have today, CALL_OPS would give us (again assuming we're using it for direct calls): * Better performance for tracer per-call (faster ops lookup) GOOD * Larger text size (function alignment + extra nops) BAD * Same direct call performance NEUTRAL * Same complicated text patching required NEUTRAL It would be interesting to see how the per-call performance would improve on x86 with CALL_OPS! ;-) I'm trying to wrap my head if it makes sense to have it on RISC-V, given that we're a bit different from Arm64. Does the scale tip to the GOOD side? Oh, and we really need to see performance numbers on real HW! I have a VF2 that I could try this series on. Björn
Re: [RFC][PATCH 1/4] module: mm: Make module_alloc() generally available
Hi Calvin, Le 06/03/2024 à 21:05, Calvin Owens a écrit : > [Vous ne recevez pas souvent de courriers de jcalvinow...@gmail.com. > Découvrez pourquoi ceci est important à > https://aka.ms/LearnAboutSenderIdentification ] > > Both BPF_JIT and KPROBES depend on CONFIG_MODULES, but only require > module_alloc() itself, which can be easily separated into a standalone > allocator for executable kernel memory. Easily maybe, but not as easily as you think, see below. > > Thomas Gleixner sent a patch to do that for x86 as part of a larger > series a couple years ago: > > https://lore.kernel.org/all/20220716230953.442937...@linutronix.de/ > > I've simply extended that approach to the whole kernel. > > Signed-off-by: Calvin Owens > --- > arch/Kconfig | 2 +- > arch/arm/kernel/module.c | 35 - > arch/arm/mm/Makefile | 2 + > arch/arm/mm/module_alloc.c | 40 ++ > arch/arm64/kernel/module.c | 127 -- > arch/arm64/mm/Makefile | 1 + > arch/arm64/mm/module_alloc.c | 130 +++ > arch/loongarch/kernel/module.c | 6 -- > arch/loongarch/mm/Makefile | 2 + > arch/loongarch/mm/module_alloc.c | 10 +++ > arch/mips/kernel/module.c| 10 --- > arch/mips/mm/Makefile| 2 + > arch/mips/mm/module_alloc.c | 13 > arch/nios2/kernel/module.c | 20 - > arch/nios2/mm/Makefile | 2 + > arch/nios2/mm/module_alloc.c | 22 ++ > arch/parisc/kernel/module.c | 12 --- > arch/parisc/mm/Makefile | 1 + > arch/parisc/mm/module_alloc.c| 15 > arch/powerpc/kernel/module.c | 36 - > arch/powerpc/mm/Makefile | 1 + > arch/powerpc/mm/module_alloc.c | 41 ++ Missing several powerpc changes to make it work. You must audit every use of CONFIG_MODULES inside powerpc. Here are a few exemples: Function get_patch_pfn() to enable text code patching. arch/powerpc/Kconfig : select KASAN_VMALLOCif KASAN && MODULES arch/powerpc/include/asm/kasan.h: #if defined(CONFIG_MODULES) && defined(CONFIG_PPC32) #define KASAN_KERN_STARTALIGN_DOWN(PAGE_OFFSET - SZ_256M, SZ_256M) #else #define KASAN_KERN_STARTPAGE_OFFSET #endif arch/powerpc/kernel/head_8xx.S and arch/powerpc/kernel/head_book3s_32.S: InstructionTLBMiss interrupt handler must know that there is executable kernel text outside kernel core. Function is_module_segment() to identified segments used for module text and set NX (NoExec) MMU flag on non-module segments. > arch/riscv/kernel/module.c | 11 --- > arch/riscv/mm/Makefile | 1 + > arch/riscv/mm/module_alloc.c | 17 > arch/s390/kernel/module.c| 37 - > arch/s390/mm/Makefile| 1 + > arch/s390/mm/module_alloc.c | 42 ++ > arch/sparc/kernel/module.c | 31 > arch/sparc/mm/Makefile | 2 + > arch/sparc/mm/module_alloc.c | 31 > arch/x86/kernel/ftrace.c | 2 +- > arch/x86/kernel/module.c | 56 - > arch/x86/mm/Makefile | 2 + > arch/x86/mm/module_alloc.c | 59 ++ > fs/proc/kcore.c | 2 +- > kernel/module/Kconfig| 1 + > kernel/module/main.c | 17 > mm/Kconfig | 3 + > mm/Makefile | 1 + > mm/module_alloc.c| 21 + > mm/vmalloc.c | 2 +- > 42 files changed, 467 insertions(+), 402 deletions(-) ... > diff --git a/mm/Kconfig b/mm/Kconfig > index ffc3a2ba3a8c..92bfb5ae2e95 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -1261,6 +1261,9 @@ config LOCK_MM_AND_FIND_VMA > config IOMMU_MM_DATA > bool > > +config MODULE_ALLOC > + def_bool n > + I'd call it something else than CONFIG_MODULE_ALLOC as you want to use it when CONFIG_MODULE is not selected. Something like CONFIG_EXECMEM_ALLOC or CONFIG_DYNAMIC_EXECMEM ? Christophe
Re: [PATCH net-next v2 0/2] tcp: add two missing addresses when using trace
Hello: This series was applied to netdev/net-next.git (main) by Paolo Abeni : On Mon, 4 Mar 2024 17:29:32 +0800 you wrote: > From: Jason Xing > > When I reviewed other people's patch [1], I noticed that similar things > also happen in tcp_event_skb class and tcp_event_sk_skb class. They > don't print those two addrs of skb/sk which already exist. > > In this patch, I just do as other trace functions do, like > trace_net_dev_start_xmit(), to know the exact flow or skb we would like > to know in case some systems doesn't support BPF programs well or we > have to use /sys/kernel/debug/tracing only for some reasons. > > [...] Here is the summary with links: - [net-next,v2,1/2] tcp: add tracing of skb/skaddr in tcp_event_sk_skb class https://git.kernel.org/netdev/net-next/c/4e441bb8aca1 - [net-next,v2,2/2] tcp: add tracing of skbaddr in tcp_event_skb class https://git.kernel.org/netdev/net-next/c/0ab544b6f055 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [RFC PATCH v3 0/7] Add virtio_rtc module and related changes
On Mon, 2023-12-18 at 08:38 +0100, Peter Hilber wrote: > RFC v3 updates > -- > > This series implements a driver for a virtio-rtc device conforming to spec > RFC v3 [1]. It now includes an RTC class driver with alarm, in addition to > the PTP clock driver already present before. > > This patch series depends on the patch series "treewide: Use clocksource id > for get_device_system_crosststamp()" [3]. Pull [4] to get the combined > series on top of mainline. > > Overview > > > This patch series adds the virtio_rtc module, and related bugfixes. The > virtio_rtc module implements a driver compatible with the proposed Virtio > RTC device specification [1]. The Virtio RTC (Real Time Clock) device > provides information about current time. The device can provide different > clocks, e.g. for the UTC or TAI time standards, or for physical time > elapsed since some past epoch. Hm, should we allow UTC? If you tell me the time in UTC, then (sometimes) I still don't actually know what the time is, because some UTC seconds occur twice. UTC only makes sense if you provide the TAI offset, surely? Should the virtio_rtc specification make it mandatory to provide such? Otherwise you're just designing it to allow crappy hypervisors to expose incomplete information. > PTP clock interface > --- > > virtio_rtc exposes clocks as PTP clocks to userspace, similar to ptp_kvm. > If both the Virtio RTC device and this driver have special support for the > current clocksource, time synchronization programs can use > cross-timestamping using ioctl PTP_SYS_OFFSET_PRECISE2 aka > PTP_SYS_OFFSET_PRECISE. Similar to ptp_kvm, system time synchronization > with single-digit ns precision is possible with a quiescent reference clock > (from the Virtio RTC device). This works even when the Virtio device > response is slow compared to ptp_kvm hypercalls. Is PTP the right mechanism for this? As I understand it, PTP is a way to precisely synchronize one clock with another. But in the case of virt guests synchronizing against the host, it isn't really *another* clock. It really is the *same* underlying clock. As the host clock varies with temperature, for example, so does the guest clock. The only difference is an offset and (on x86 perhaps) a mathematical scaling of the frequency. I was looking at this another way, when I came across this virtio-rtc work. My idea was just for the hypervisor to expose its own timekeeping information — the counter/TSC value and TAI time at a given moment, frequency of the counter, and the precision of both that frequency (±PPM) and the TAI timestamp (±µs). By putting that in a host/guest shared data structure with a seqcount for lockless updates, we can update it as time synchronization on the host is refined, and we can even cleanly handle live migration where the guest ends up on a completely different host. It allows for use cases which *really* care (e.g. timestamping financial transactions) to ensure that there is never even a moment of getting *wrong* timestamps if they haven't yet resynced after a migration. Now I'm trying to work out if I should attempt to reconcile with your existing virtio-rtc work, or just decide that virtio-rtc isn't trying to solve the actual problem that we have, and go ahead with something different... ? smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH net-next v2 2/2] tcp: add tracing of skbaddr in tcp_event_skb class
On Mon, Mar 4, 2024 at 10:29 AM Jason Xing wrote: > > From: Jason Xing > > Use the existing parameter and print the address of skbaddr > as other trace functions do. > > Signed-off-by: Jason Xing Reviewed-by: Eric Dumazet
Re: [PATCH net-next v2 1/2] tcp: add tracing of skb/skaddr in tcp_event_sk_skb class
On Mon, Mar 4, 2024 at 10:29 AM Jason Xing wrote: > > From: Jason Xing > > Printing the addresses can help us identify the exact skb/sk > for those system in which it's not that easy to run BPF program. > As we can see, it already fetches those, then use it directly > and it will print like below: > > ...tcp_retransmit_skb: skbaddr=XXX skaddr=XXX family=AF_INET... > > Signed-off-by: Jason Xing Reviewed-by: Eric Dumazet
Re: [PATCH -next v6 0/2] Make memory reclamation measurable
On Thu 07-03-24 15:40:29, Bixuan Cui wrote: [...] > Currently, with the help of kernel trace events or tools like Perfetto, we > can only see that kswapd is competing for CPU and the frequency of memory > reclamation triggers, but we do not have detailed information or metrics > about memory reclamation, such as the duration and amount of each > reclamation, or who is releasing memory (super_cache, f2fs, ext4), etc. This > makes it impossible to locate the above problems. I am not sure I agree with you here. We do provide insight into LRU and shrinkers reclaim. Why isn't that enough. In general I would advise you to focus more on describing why the existing infrastructure is insuficient (having examples would be really appreciated). > Currently this patch helps us solve 2 actual performance problems (kswapd > preempts the CPU causing game delay) > 1. The increased memory allocation in the game (across different versions) > has led to the degradation of kswapd. > This is found by calculating the total amount of Reclaim(page) during > the game startup phase. > > 2. The adoption of a different file system in the new system version has > resulted in a slower reclamation rate. > This is discovered through the OBJ_NAME change. For example, OBJ_NAME > changes from super_cache_scan to ext4_es_scan. > > Subsequently, it is also possible to calculate the memory reclamation rate > to evaluate the memory performance of different versions. Why cannot you achive this with existing tracing or /proc/vmstat infrastructure? > The main reasons for adding static tracepoints are: > 1. To subdivide the time spent in the shrinker->count_objects() and > shrinker->scan_objects() functions within the do_shrink_slab function. Using > BPF kprobe, we can only track the time spent in the do_shrink_slab function. > 2. When tracing frequently called functions, static tracepoints (BPF > tp/tracepoint) have lower performance impact compared to dynamic tracepoints > (BPF kprobe). You can track the time process has been preempted by other means, no? We have context switching tracepoints in place. Have you considered that option? -- Michal Hocko SUSE Labs
Re: [RFC PATCH v3 2/5] mfd: add driver for Marvell 88PM886 PMIC
> > > +static int pm886_initialize_subregmaps(struct pm886_chip *chip)
> > > +{
> > > + struct device *dev = >client->dev;
> > > + struct i2c_client *page;
> > > + struct regmap *regmap;
> > > + int err;
> > > +
> > > + /* regulators page */
> > > + page = devm_i2c_new_dummy_device(dev, chip->client->adapter,
> > > + chip->client->addr +
> > > PM886_PAGE_OFFSET_REGULATORS);
> > > + if (IS_ERR(page)) {
> > > + err = PTR_ERR(page);
> > > + dev_err(dev, "Failed to initialize regulators client: %d\n",
> > > err);
> > > + return err;
> > > + }
> > > + regmap = devm_regmap_init_i2c(page, _i2c_regmap);
> > > + if (IS_ERR(regmap)) {
> > > + err = PTR_ERR(regmap);
> > > + dev_err(dev, "Failed to initialize regulators regmap: %d\n",
> > > err);
> > > + return err;
> > > + }
> > > + chip->regmaps[PM886_REGMAP_REGULATORS] = regmap;
> >
> > Except for the regulator driver, where else is the regulators regmap used?
>
> Nowhere, at least as of now. So you are saying that I should initialize
> the regmap in the regulator driver?
I am.
--
Lee Jones [李琼斯]
Re: [PATCH v3 2/3] dt-bindings: input: imagis: Document touch keys
On 06/03/2024 15:40, Duje Mihanović wrote: > IST3032C (and possibly some other models) has touch keys. Document this. > > Signed-off-by: Duje Mihanović > --- Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
