Re: [PATCH 20/24] rcu/tree: Make kvfree_rcu() tolerate any alignment

From: "Paul E. McKenney" <paulmck@kernel.org>
To: "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	Matthew Wilcox <willy@infradead.org>,
	Joel Fernandes <joel@joelfernandes.org>,
	RCU <rcu@vger.kernel.org>,
	Oleksiy Avramchenko <oleksiy.avramchenko@sonymobile.com>
Subject: Re: [PATCH 20/24] rcu/tree: Make kvfree_rcu() tolerate any alignment
Date: Fri, 1 May 2020 16:00:52 -0700	[thread overview]
Message-ID: <20200501230052.GG7560@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200428205903.61704-21-urezki@gmail.com>

On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote:
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> 
> Handle cases where the the object being kvfree_rcu()'d is not aligned by
> 2-byte boundaries.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 501cac02146d..649bad7ad0f0 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data {
>  #define KVFREE_BULK_MAX_ENTR \
>  	((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *))
>  
> +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */
> +#define RCU_HEADLESS_KFREE BIT(31)

Did I miss the check for freeing something larger than 2GB?  Or is this
impossible, even on systems with many terabytes of physical memory?
Even if it is currently impossible, what prevents it from suddenly
becoming all too possible at some random point in the future?  If you
think that this will never happen, please keep in mind that the first
time I heard "640K ought to be enough for anybody", it sounded eminently
reasonable to me.

Besides...

Isn't the offset in question the offset of an rcu_head struct within
the enclosing structure?  If so, why not keep the current requirement
that this be at least 16-bit aligned, especially given that some work
is required to make that alignment less than pointer sized?  Then you
can continue using bit 0.

This alignment requirement is included in the RCU requirements
documentation and is enforced within the __call_rcu() function.

So let's leave this at bit 0.

							Thanx, Paul

>  /**
>   * struct kfree_rcu_cpu_work - single batch of kfree_rcu() requests
>   * @rcu_work: Let queue_rcu_work() invoke workqueue handler after grace period
> @@ -3078,9 +3081,9 @@ static void kfree_rcu_work(struct work_struct *work)
>  		next = head->next;
>  
>  		/* We tag the headless object, if so adjust offset. */
> -		headless = (((unsigned long) head - offset) & BIT(0));
> +		headless = !!(offset & RCU_HEADLESS_KFREE);
>  		if (headless)
> -			offset -= 1;
> +			offset &= ~(RCU_HEADLESS_KFREE);
>  
>  		ptr = (void *) head - offset;
>  
> @@ -3356,7 +3359,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  			 * that has to be freed as well as dynamically
>  			 * attached wrapper/head.
>  			 */
> -			func = (rcu_callback_t) (sizeof(unsigned long *) + 1);
> +			func = (rcu_callback_t)(sizeof(unsigned long *) | RCU_HEADLESS_KFREE);
>  		}
>  
>  		head->func = func;
> -- 
> 2.20.1
> 