| Message ID | 20200428205903.61704-22-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce kvfree_rcu(1 or 2 arguments) | expand |
On Tue, Apr 28, 2020 at 10:59:00PM +0200, Uladzislau Rezki (Sony) wrote: > Move inlined kvfree_call_rcu() function out of the > header file. This step is a preparation for head-less > support. > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > include/linux/rcutiny.h | 6 +----- > kernel/rcu/tiny.c | 6 ++++++ > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > index 0c6315c4a0fe..7eb66909ae1b 100644 > --- a/include/linux/rcutiny.h > +++ b/include/linux/rcutiny.h > @@ -34,11 +34,7 @@ static inline void synchronize_rcu_expedited(void) > synchronize_rcu(); > } > > -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > -{ > - call_rcu(head, func); > -} > - > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); > void rcu_qs(void); > > static inline void rcu_softirq_qs(void) > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c > index aa897c3f2e92..508c82faa45c 100644 > --- a/kernel/rcu/tiny.c > +++ b/kernel/rcu/tiny.c > @@ -177,6 +177,12 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func) > } > EXPORT_SYMBOL_GPL(call_rcu); > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > +{ > + call_rcu(head, func); > +} > +EXPORT_SYMBOL_GPL(kvfree_call_rcu); This increases the size of Tiny RCU. Plus in Tiny RCU, the overhead of synchronize_rcu() is exactly zero. So why not make the single-argument kvfree_call_rcu() just unconditionally do synchronize_rcu() followed by kvfree() or whatever? That should go just fine into the header file. Thanx, Paul > void __init rcu_init(void) > { > open_softirq(RCU_SOFTIRQ, rcu_process_callbacks); > -- > 2.20.1 >
On Fri, May 01, 2020 at 04:03:59PM -0700, Paul E. McKenney wrote: > On Tue, Apr 28, 2020 at 10:59:00PM +0200, Uladzislau Rezki (Sony) wrote: > > Move inlined kvfree_call_rcu() function out of the > > header file. This step is a preparation for head-less > > support. > > > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > --- > > include/linux/rcutiny.h | 6 +----- > > kernel/rcu/tiny.c | 6 ++++++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > > index 0c6315c4a0fe..7eb66909ae1b 100644 > > --- a/include/linux/rcutiny.h > > +++ b/include/linux/rcutiny.h > > @@ -34,11 +34,7 @@ static inline void synchronize_rcu_expedited(void) > > synchronize_rcu(); > > } > > > > -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > -{ > > - call_rcu(head, func); > > -} > > - > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); > > void rcu_qs(void); > > > > static inline void rcu_softirq_qs(void) > > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c > > index aa897c3f2e92..508c82faa45c 100644 > > --- a/kernel/rcu/tiny.c > > +++ b/kernel/rcu/tiny.c > > @@ -177,6 +177,12 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func) > > } > > EXPORT_SYMBOL_GPL(call_rcu); > > > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > +{ > > + call_rcu(head, func); > > +} > > +EXPORT_SYMBOL_GPL(kvfree_call_rcu); > > This increases the size of Tiny RCU. Plus in Tiny RCU, the overhead of > synchronize_rcu() is exactly zero. So why not make the single-argument > kvfree_call_rcu() just unconditionally do synchronize_rcu() followed by > kvfree() or whatever? That should go just fine into the header file. > I was thinking about it. That makes sense. Let me rework it then :) -- Vlad Rezki
Hello, Paul, Joel. > > Move inlined kvfree_call_rcu() function out of the > > header file. This step is a preparation for head-less > > support. > > > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > --- > > include/linux/rcutiny.h | 6 +----- > > kernel/rcu/tiny.c | 6 ++++++ > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > > index 0c6315c4a0fe..7eb66909ae1b 100644 > > --- a/include/linux/rcutiny.h > > +++ b/include/linux/rcutiny.h > > @@ -34,11 +34,7 @@ static inline void synchronize_rcu_expedited(void) > > synchronize_rcu(); > > } > > > > -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > -{ > > - call_rcu(head, func); > > -} > > - > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); > > void rcu_qs(void); > > > > static inline void rcu_softirq_qs(void) > > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c > > index aa897c3f2e92..508c82faa45c 100644 > > --- a/kernel/rcu/tiny.c > > +++ b/kernel/rcu/tiny.c > > @@ -177,6 +177,12 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func) > > } > > EXPORT_SYMBOL_GPL(call_rcu); > > > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > +{ > > + call_rcu(head, func); > > +} > > +EXPORT_SYMBOL_GPL(kvfree_call_rcu); > > This increases the size of Tiny RCU. Plus in Tiny RCU, the overhead of > synchronize_rcu() is exactly zero. So why not make the single-argument > kvfree_call_rcu() just unconditionally do synchronize_rcu() followed by > kvfree() or whatever? That should go just fine into the header file. > Seems it does not go well if i do it in header file: <snip> diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h index 0c6315c4a0fe..76b7ad053218 100644 --- a/include/linux/rcutiny.h +++ b/include/linux/rcutiny.h @@ -13,6 +13,7 @@ #define __LINUX_TINY_H #include <asm/param.h> /* for HZ */ +#include <linux/mm.h> /* Never flag non-existent other CPUs! */ static inline bool rcu_eqs_special_set(int cpu) { return false; } @@ -36,7 +37,15 @@ static inline void synchronize_rcu_expedited(void) static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) { - call_rcu(head, func); + if (head) { + call_rcu(head, func); + return; + } + + // kvfree_rcu(one_arg) call. + might_sleep(); + synchronize_rcu(); + kvfree((void *) func); } <snip> kvfree() is defined in <linux/mm.h> as extern void kvfree(const void *addr); If i just include <linux/mm.h> i get many errors related to "implicit declaration of function" like: <snip> rcu_read_lock() compound_mapcount_ptr() rcu_assign_pointer() ... <snip> and many other messages like: <snip> warning: returning ‘int’ from a function with return type error: unknown type name ‘vm_fault_t’; did you mean ‘pmdval_t’? error: implicit declaration of function ‘RB_EMPTY_ROOT’ ... <snip> Please see full log here: ftp://vps418301.ovh.net/incoming/include_mm_h_output.txt I can fix it by adding the kvfree() declaration to the rcutiny.h also: extern void kvfree(const void *addr); what seems wired to me? Also it can be fixed if i move it to the tiny.c so it will be aligned with the way how it is done for tree-RCU. Any valuable proposals? -- Vlad Rezki
On Wed, May 06, 2020 at 08:29:02PM +0200, Uladzislau Rezki wrote: > Hello, Paul, Joel. > > > > Move inlined kvfree_call_rcu() function out of the > > > header file. This step is a preparation for head-less > > > support. > > > > > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > --- > > > include/linux/rcutiny.h | 6 +----- > > > kernel/rcu/tiny.c | 6 ++++++ > > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > > > index 0c6315c4a0fe..7eb66909ae1b 100644 > > > --- a/include/linux/rcutiny.h > > > +++ b/include/linux/rcutiny.h > > > @@ -34,11 +34,7 @@ static inline void synchronize_rcu_expedited(void) > > > synchronize_rcu(); > > > } > > > > > > -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > -{ > > > - call_rcu(head, func); > > > -} > > > - > > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); > > > void rcu_qs(void); > > > > > > static inline void rcu_softirq_qs(void) > > > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c > > > index aa897c3f2e92..508c82faa45c 100644 > > > --- a/kernel/rcu/tiny.c > > > +++ b/kernel/rcu/tiny.c > > > @@ -177,6 +177,12 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func) > > > } > > > EXPORT_SYMBOL_GPL(call_rcu); > > > > > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > +{ > > > + call_rcu(head, func); > > > +} > > > +EXPORT_SYMBOL_GPL(kvfree_call_rcu); > > > > This increases the size of Tiny RCU. Plus in Tiny RCU, the overhead of > > synchronize_rcu() is exactly zero. So why not make the single-argument > > kvfree_call_rcu() just unconditionally do synchronize_rcu() followed by > > kvfree() or whatever? That should go just fine into the header file. > > > Seems it does not go well if i do it in header file: > > <snip> > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > index 0c6315c4a0fe..76b7ad053218 100644 > --- a/include/linux/rcutiny.h > +++ b/include/linux/rcutiny.h > @@ -13,6 +13,7 @@ > #define __LINUX_TINY_H > > #include <asm/param.h> /* for HZ */ > +#include <linux/mm.h> > > /* Never flag non-existent other CPUs! */ > static inline bool rcu_eqs_special_set(int cpu) { return false; } > @@ -36,7 +37,15 @@ static inline void synchronize_rcu_expedited(void) > > static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > { > - call_rcu(head, func); > + if (head) { > + call_rcu(head, func); > + return; > + } > + > + // kvfree_rcu(one_arg) call. > + might_sleep(); > + synchronize_rcu(); > + kvfree((void *) func); > } > <snip> > > kvfree() is defined in <linux/mm.h> as extern void kvfree(const void *addr); > If i just include <linux/mm.h> i get many errors related to "implicit declaration > of function" like: > > <snip> > rcu_read_lock() > compound_mapcount_ptr() > rcu_assign_pointer() > ... > <snip> > > and many other messages like: > > <snip> > warning: returning ‘int’ from a function with return type > error: unknown type name ‘vm_fault_t’; did you mean ‘pmdval_t’? > error: implicit declaration of function ‘RB_EMPTY_ROOT’ > ... > <snip> > > Please see full log here: ftp://vps418301.ovh.net/incoming/include_mm_h_output.txt > > I can fix it by adding the kvfree() declaration to the rcutiny.h also: > extern void kvfree(const void *addr); > > what seems wired to me? Also it can be fixed if i move it to the tiny.c > so it will be aligned with the way how it is done for tree-RCU. If the mm guys are OK with the kvfree() declaration, that is the way to go. With the addition of a comment saying something like "Avoid #include hell". The compiler will complain if the definition changes given that there has to be somewhere that sees both the above and the real declaration, so this should not cause too much trouble. > Any valuable proposals? Otherwise, yes, the function would need to move to tiny.c and thus add bloat. :-( Thanx, Paul
> > > > Please see full log here: ftp://vps418301.ovh.net/incoming/include_mm_h_output.txt > > > > I can fix it by adding the kvfree() declaration to the rcutiny.h also: > > extern void kvfree(const void *addr); > > > > what seems wired to me? Also it can be fixed if i move it to the tiny.c > > so it will be aligned with the way how it is done for tree-RCU. > > If the mm guys are OK with the kvfree() declaration, that is the way > to go. With the addition of a comment saying something like "Avoid > #include hell". > > The compiler will complain if the definition changes given that there > has to be somewhere that sees both the above and the real declaration, > so this should not cause too much trouble. > > > Any valuable proposals? > > Otherwise, yes, the function would need to move to tiny.c and thus add > bloat. :-( > OK. I will declare it one more time. Indeed if it is changed, the compiler will emit some errors. Also, i will add some comments why it is done. Thanks! -- Vlad Rezki
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h index 0c6315c4a0fe..7eb66909ae1b 100644 --- a/include/linux/rcutiny.h +++ b/include/linux/rcutiny.h @@ -34,11 +34,7 @@ static inline void synchronize_rcu_expedited(void) synchronize_rcu(); } -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) -{ - call_rcu(head, func); -} - +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); void rcu_qs(void); static inline void rcu_softirq_qs(void) diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c index aa897c3f2e92..508c82faa45c 100644 --- a/kernel/rcu/tiny.c +++ b/kernel/rcu/tiny.c @@ -177,6 +177,12 @@ void call_rcu(struct rcu_head *head, rcu_callback_t func) } EXPORT_SYMBOL_GPL(call_rcu); +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) +{ + call_rcu(head, func); +} +EXPORT_SYMBOL_GPL(kvfree_call_rcu); + void __init rcu_init(void) { open_softirq(RCU_SOFTIRQ, rcu_process_callbacks);