linux-kernel - Re: [PATCH 21/24] rcu/tiny: move kvfree_call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200506184548.GE2869@paulmck-ThinkPad-P72>
Date:   Wed, 6 May 2020 11:45:48 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Uladzislau Rezki <urezki@...il.com>
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Matthew Wilcox <willy@...radead.org>,
        RCU <rcu@...r.kernel.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>
Subject: Re: [PATCH 21/24] rcu/tiny: move kvfree_call_rcu() out of header

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@...lfernandes.org>
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@...il.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