linux-kernel - Re: [PATCH 1/3] rcu/tree: use more permissive parameters when attaching a head

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] [day] [month] [year] [list]

Date:   Mon, 6 Apr 2020 22:11:18 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Uladzislau Rezki (Sony)" <urezki@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        RCU <rcu@...r.kernel.org>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>
Subject: Re: [PATCH 1/3] rcu/tree: use more permissive parameters when
 attaching a head

On Thu, Apr 02, 2020 at 02:32:51PM +0200, Uladzislau Rezki (Sony) wrote:
> It is documneted that a headless object can be reclaimed from
> might_sleep() context only. Because of that when a head is
> dynamically attached it makes sense to drop the lock and do
> an allocation with much more permissve flags comparing if it
> is done from atomic context.
> 
> That is why use GFP_KERNEL flag plus some extra ones which
> would make an allocation most likely to be succeed. The big
> advantage of doing so is a direct reclaim process.
> 
> Tested such approach on my local tiny system with 145MB of
> ram(the minimum amount the KVM system is capable of booting)
> and 4xCPUs. For stressing the rcuperf module was used. During
> tests with difference combinations i did not observe any hit
> of our last emergency case, when synchronize_rcu() is involved.
> 
> Please note, the "dynamically attaching" path was enabled only,
> apart of that all types of objects were considered as headless
> variant during testing.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@...il.com>
> Suggested-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> ---
>  kernel/rcu/tree.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 6172e6296dd7..24f620a06219 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3148,13 +3148,10 @@ static inline struct rcu_head *attach_rcu_head_to_object(void *obj)
>  {
>  	unsigned long *ptr;
>  
> +	/* Try hard to get the memory. */
>  	ptr = kmalloc(sizeof(unsigned long *) +
> -			sizeof(struct rcu_head), GFP_NOWAIT | __GFP_NOWARN);
> -
> -	if (!ptr)
> -		ptr = kmalloc(sizeof(unsigned long *) +
> -				sizeof(struct rcu_head), GFP_ATOMIC | __GFP_NOWARN);
> -
> +		sizeof(struct rcu_head), GFP_KERNEL |
> +			__GFP_ATOMIC | __GFP_HIGH | __GFP_RETRY_MAYFAIL);

On thing here though, you removed the NOWARN. Was there a reason? It would
now warn even when synchronously waiting right? I will fixup your commit to
add it back for now but let me know if you had some other reason to remove it.

thanks,

 - Joel


>  	if (!ptr)
>  		return NULL;
>  
> @@ -3222,9 +3219,20 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  	if (!success) {
>  		/* Is headless object? */
>  		if (head == NULL) {
> +			/* Drop the lock. */
> +			if (krcp->initialized)
> +				spin_unlock(&krcp->lock);
> +			local_irq_restore(flags);
> +
>  			head = attach_rcu_head_to_object(ptr);
>  			if (head == NULL)
> -				goto unlock_return;
> +				goto inline_return;
> +
> +			/* Take it back. */
> +			local_irq_save(flags);
> +			krcp = this_cpu_ptr(&krc);
> +			if (krcp->initialized)
> +				spin_lock(&krcp->lock);
>  
>  			/*
>  			 * Tag the headless object. Such objects have a back-pointer
> @@ -3263,6 +3271,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  		spin_unlock(&krcp->lock);
>  	local_irq_restore(flags);
>  
> +inline_return:
>  	/*
>  	 * High memory pressure, so inline kvfree() after
>  	 * synchronize_rcu(). We can do it from might_sleep()
> -- 
> 2.20.1
>