linux-kernel - Re: [PATCH 1/2] Move kfree_call_rcu() to slab

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: <20180102222341.GB20405@bombadil.infradead.org>
Date:   Tue, 2 Jan 2018 14:23:41 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     rao.shoaib@...cle.com
Cc:     linux-kernel@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
        brouer@...hat.com, linux-mm@...ck.org
Subject: Re: [PATCH 1/2] Move kfree_call_rcu() to slab_common.c

On Tue, Jan 02, 2018 at 12:11:37PM -0800, rao.shoaib@...cle.com wrote:
> -#define kfree_rcu(ptr, rcu_head)					\
> -	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))

> +#define kfree_rcu(ptr, rcu_head_name)	\
> +	do { \
> +		typeof(ptr) __ptr = ptr;	\
> +		unsigned long __off = offsetof(typeof(*(__ptr)), \
> +						      rcu_head_name); \
> +		struct rcu_head *__rptr = (void *)__ptr + __off; \
> +		__kfree_rcu(__rptr, __off); \
> +	} while (0)

I feel like you're trying to help people understand the code better,
but using longer names can really work against that.  Reverting to
calling the parameter 'rcu_head' lets you not split the line:

+#define kfree_rcu(ptr, rcu_head)	\
+	do { \
+		typeof(ptr) __ptr = ptr;	\
+		unsigned long __off = offsetof(typeof(*(__ptr)), rcu_head); \
+		struct rcu_head *__rptr = (void *)__ptr + __off; \
+		__kfree_rcu(__rptr, __off); \
+	} while (0)

Also, I don't understand why you're bothering to create __ptr here.
I understand the desire to not mention the same argument more than once,
but you have 'ptr' twice anyway.

And it's good practice to enclose macro arguments in parentheses in case
the user has done something really tricksy like pass in "p + 1".

In summary, I don't see anything fundamentally better in your rewrite
of kfree_rcu().  The previous version is more succinct, and to my
mind, easier to understand.

> +void call_rcu_lazy(struct rcu_head *head, rcu_callback_t func)
> +{
> +	__call_rcu(head, func, &rcu_sched_state, -1, 1);
> +}

> -void kfree_call_rcu(struct rcu_head *head,
> -		    rcu_callback_t func)
> -{
> -	__call_rcu(head, func, rcu_state_p, -1, 1);
> -}

You've silently changed this.  Why?  It might well be the right change,
but it at least merits mentioning in the changelog.