[PATCH RFC] netfilter: cacheline aligning in struct netns_ct

[PATCH RFC] netfilter: cacheline aligning in struct netns_ct

[Zhouyi Zhou]

not frequently changing components should share same cachelines

Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
---
 include/net/netns/conntrack.h |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index fbcc7fa..69d2d58 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h
@@ -65,6 +65,12 @@ struct nf_ip_net {
 struct netns_ct {
 	atomic_t		count;
 	unsigned int		expect_count;
+	struct hlist_nulls_head	unconfirmed;
+	struct hlist_nulls_head	dying;
+	struct hlist_nulls_head tmpl;
+
+	/*  not frequently changing components should share same cachelines */
+	struct nf_ip_net	nf_ct_proto ____cacheline_aligned_in_smp;
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header	*sysctl_header;
 	struct ctl_table_header	*acct_sysctl_header;
@@ -86,13 +92,11 @@ struct netns_ct {
 	struct kmem_cache	*nf_conntrack_cachep;
 	struct hlist_nulls_head	*hash;
 	struct hlist_head	*expect_hash;
-	struct hlist_nulls_head	unconfirmed;
-	struct hlist_nulls_head	dying;
-	struct hlist_nulls_head tmpl;
+
 	struct ip_conntrack_stat __percpu *stat;
 	struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;
 	struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
-	struct nf_ip_net	nf_ct_proto;
+
 #if defined(CONFIG_NF_CONNTRACK_LABELS)
 	unsigned int		labels_used;
 	u8			label_words;
-- 
1.7.10.4

Re: [PATCH RFC] netfilter: cacheline aligning in struct netns_ct

[Jesper Dangaard Brouer]

On Wed, 12 Mar 2014 15:56:34 +0800
Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:

> not frequently changing components should share same cachelines
> 
> Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
> ---
>  include/net/netns/conntrack.h |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

NACK because I have already moved these "special" lists in commit:

https://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/commit/?id=b7779d06f9950e14a008a2de970b44233fe49c86

Next remember that this struct netns_ct, is inlined in struct net
(include/net/net_namespace.h).  Thus, the alignment is not quite as you
might expect...

> diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
> index fbcc7fa..69d2d58 100644
> --- a/include/net/netns/conntrack.h
> +++ b/include/net/netns/conntrack.h
> @@ -65,6 +65,12 @@ struct nf_ip_net {
>  struct netns_ct {
>  	atomic_t		count;
>  	unsigned int		expect_count;
> +	struct hlist_nulls_head	unconfirmed;
> +	struct hlist_nulls_head	dying;
> +	struct hlist_nulls_head tmpl;
> +
> +	/*  not frequently changing components should share same cachelines */
> +	struct nf_ip_net	nf_ct_proto ____cacheline_aligned_in_smp;
>  #ifdef CONFIG_SYSCTL
>  	struct ctl_table_header	*sysctl_header;
>  	struct ctl_table_header	*acct_sysctl_header;
> @@ -86,13 +92,11 @@ struct netns_ct {
>  	struct kmem_cache	*nf_conntrack_cachep;
>  	struct hlist_nulls_head	*hash;
>  	struct hlist_head	*expect_hash;
> -	struct hlist_nulls_head	unconfirmed;
> -	struct hlist_nulls_head	dying;
> -	struct hlist_nulls_head tmpl;
> +
>  	struct ip_conntrack_stat __percpu *stat;
>  	struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;
>  	struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
> -	struct nf_ip_net	nf_ct_proto;
> +
>  #if defined(CONFIG_NF_CONNTRACK_LABELS)
>  	unsigned int		labels_used;
>  	u8			label_words;



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

RE: [PATCH RFC] netfilter: cacheline aligning in struct netns_ct

[David Laight]

From: Zhouyi Zhou
> not frequently changing components should share same cachelines
> 
> Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
> ---
>  include/net/netns/conntrack.h |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
> index fbcc7fa..69d2d58 100644
> --- a/include/net/netns/conntrack.h
> +++ b/include/net/netns/conntrack.h
> @@ -65,6 +65,12 @@ struct nf_ip_net {
>  struct netns_ct {
>  	atomic_t		count;
>  	unsigned int		expect_count;
> +	struct hlist_nulls_head	unconfirmed;
> +	struct hlist_nulls_head	dying;
> +	struct hlist_nulls_head tmpl;
> +
> +	/*  not frequently changing components should share same cachelines */
> +	struct nf_ip_net	nf_ct_proto ____cacheline_aligned_in_smp;

I'm not sure this has the effect you are trying to achieve.
Adding an __attribute__((aligned(n))) to a structure member not only
adds padding before the member, but also pads the member to a multiple
of the specified size.

With large cache lines you probably want neither.
IIRC one of the ppc systems has something like 256 byte cache lines
(and if it doesn't now, it might have soon).
So arbitrarily cache-line aligning structure members may not be a good idea.

In any case reducing the number of cache lines accessed, and the number
dirtied is probably more important than putting 'readonly' data in its
own cache lines.

	David

Re: [PATCH RFC] netfilter: cacheline aligning in struct netns_ct

[Zhouyi Zhou]

Thanks Jesper and David for reviewing and replying.

Great commitment of Jesper for moving above three hlist_nulls_heads to
percpu structure. Sorry to miss the newest advances in nf-next.

And arbitrarily cache-line aligning structure members is not a good idea
for large cache lines.

Still I have an idea that could we pack the seldom changed members
 (nf_ct_proto included) to a structure and we allocated it from a
cache aligned slub when the structure "net" is initialized.


Zhouyi

On Wed, Mar 12, 2014 at 5:36 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Zhouyi Zhou
>> not frequently changing components should share same cachelines
>>
>> Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
>> ---
>>  include/net/netns/conntrack.h |   12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
>> index fbcc7fa..69d2d58 100644
>> --- a/include/net/netns/conntrack.h
>> +++ b/include/net/netns/conntrack.h
>> @@ -65,6 +65,12 @@ struct nf_ip_net {
>>  struct netns_ct {
>>       atomic_t                count;
>>       unsigned int            expect_count;
>> +     struct hlist_nulls_head unconfirmed;
>> +     struct hlist_nulls_head dying;
>> +     struct hlist_nulls_head tmpl;
>> +
>> +     /*  not frequently changing components should share same cachelines */
>> +     struct nf_ip_net        nf_ct_proto ____cacheline_aligned_in_smp;
>
> I'm not sure this has the effect you are trying to achieve.
> Adding an __attribute__((aligned(n))) to a structure member not only
> adds padding before the member, but also pads the member to a multiple
> of the specified size.
>
> With large cache lines you probably want neither.
> IIRC one of the ppc systems has something like 256 byte cache lines
> (and if it doesn't now, it might have soon).
> So arbitrarily cache-line aligning structure members may not be a good idea.
>
> In any case reducing the number of cache lines accessed, and the number
> dirtied is probably more important than putting 'readonly' data in its
> own cache lines.
>
>         David
>
>
>