[Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

On x86_64, before this patch:

struct tcp_fastopen_metrics {
	u16                        mss;                  /*     0     2 */
	u16                        syn_loss:10;          /*     2: 6  2 */

	/* XXX 6 bits hole, try to pack */
	/* XXX 4 bytes hole, try to pack */

	long unsigned int          last_syn_loss;        /*     8     8 */
	struct tcp_fastopen_cookie cookie;               /*    16    17 */

	/* size: 40, cachelines: 1, members: 4 */
	/* sum members: 29, holes: 1, sum holes: 4 */
	/* bit holes: 1, sum bit holes: 6 bits */
	/* padding: 7 */
	/* last cacheline: 40 bytes */
};

after this patch:

struct tcp_fastopen_metrics {
	u16                        mss;                  /*     0     2 */
	u16                        syn_loss:10;          /*     2: 6  2 */

	/* XXX 6 bits hole, try to pack */

	struct tcp_fastopen_cookie cookie;               /*     4    17 */

	/* XXX 3 bytes hole, try to pack */

	long unsigned int          last_syn_loss;        /*    24     8 */

	/* size: 32, cachelines: 1, members: 4 */
	/* sum members: 29, holes: 1, sum holes: 3 */
	/* bit holes: 1, sum bit holes: 6 bits */
	/* last cacheline: 32 bytes */
};

On 32bit, the 4-byte hole should not exist, so this patch probably
doesn't change anything.

Cc: Eric Dumazet <edumazet@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 10b3796..438393f 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -25,8 +25,8 @@ int sysctl_tcp_nometrics_save __read_mostly;
 struct tcp_fastopen_metrics {
 	u16	mss;
 	u16	syn_loss:10;		/* Recurring Fast Open SYN losses */
-	unsigned long	last_syn_loss;	/* Last Fast Open SYN loss */
 	struct	tcp_fastopen_cookie	cookie;
+	unsigned long	last_syn_loss;	/* Last Fast Open SYN loss */
 };
 
 struct tcp_metrics_block {

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Eric Dumazet]

On Wed, 2013-07-31 at 10:48 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
> 
> On x86_64, before this patch:
> 
> struct tcp_fastopen_metrics {
> 	u16                        mss;                  /*     0     2 */
> 	u16                        syn_loss:10;          /*     2: 6  2 */
> 
> 	/* XXX 6 bits hole, try to pack */
> 	/* XXX 4 bytes hole, try to pack */
> 
> 	long unsigned int          last_syn_loss;        /*     8     8 */
> 	struct tcp_fastopen_cookie cookie;               /*    16    17 */
> 
> 	/* size: 40, cachelines: 1, members: 4 */
> 	/* sum members: 29, holes: 1, sum holes: 4 */
> 	/* bit holes: 1, sum bit holes: 6 bits */
> 	/* padding: 7 */
> 	/* last cacheline: 40 bytes */
> };
> 
> after this patch:
> 
> struct tcp_fastopen_metrics {
> 	u16                        mss;                  /*     0     2 */
> 	u16                        syn_loss:10;          /*     2: 6  2 */
> 
> 	/* XXX 6 bits hole, try to pack */
> 
> 	struct tcp_fastopen_cookie cookie;               /*     4    17 */
> 
> 	/* XXX 3 bytes hole, try to pack */
> 
> 	long unsigned int          last_syn_loss;        /*    24     8 */
> 
> 	/* size: 32, cachelines: 1, members: 4 */
> 	/* sum members: 29, holes: 1, sum holes: 3 */
> 	/* bit holes: 1, sum bit holes: 6 bits */
> 	/* last cacheline: 32 bytes */
> };
> 
> On 32bit, the 4-byte hole should not exist, so this patch probably
> doesn't change anything.
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>

Oh well, this patch is pure noise...

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

On Tue, 2013-07-30 at 20:08 -0700, Eric Dumazet wrote:
> 
> Oh well, this patch is pure noise...
> 
> 

Mind to be specific?

I know saving 8 bytes is not interesting for you, but it is for me,
since I need some room in struct tcp_metrics_block for union inet_addr.
With this patch, I don't have to make struct tcp_metrics_block expand to
3 cachelines. :)

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Eric Dumazet]

On Wed, 2013-07-31 at 11:13 +0800, Cong Wang wrote:
> On Tue, 2013-07-30 at 20:08 -0700, Eric Dumazet wrote:
> > 
> > Oh well, this patch is pure noise...
> > 
> > 
> 
> Mind to be specific?
> 
> I know saving 8 bytes is not interesting for you, but it is for me,
> since I need some room in struct tcp_metrics_block for union inet_addr.
> With this patch, I don't have to make struct tcp_metrics_block expand to
> 3 cachelines. :)

Do you see how this explanation is rather different than the one you
gave in the changelog ?

Its 3 lines, instead of all this pahole noise.

And it would be more logical to put the "unsigned long last_syn_loss" at
the beginning of the structure, instead after an array of 17 bytes.

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Joe Perches]

On Tue, 2013-07-30 at 20:24 -0700, Eric Dumazet wrote:
> On Wed, 2013-07-31 at 11:13 +0800, Cong Wang wrote:
> > On Tue, 2013-07-30 at 20:08 -0700, Eric Dumazet wrote:
> > > Oh well, this patch is pure noise...
> > Mind to be specific?
> > 
> > I know saving 8 bytes is not interesting for you, but it is for me,
> > since I need some room in struct tcp_metrics_block for union inet_addr.
> > With this patch, I don't have to make struct tcp_metrics_block expand to
> > 3 cachelines. :)
> 
> Do you see how this explanation is rather different than the one you
> gave in the changelog ?

Eric, do you see how your own original reply was unwarranted?

> And it would be more logical to put the "unsigned long last_syn_loss" at
> the beginning of the structure, instead after an array of 17 bytes.

True, using "unsigned long" instead of "long unsigned int"
would likely be better too.

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

On Tue, 2013-07-30 at 20:24 -0700, Eric Dumazet wrote:
> On Wed, 2013-07-31 at 11:13 +0800, Cong Wang wrote:
> > On Tue, 2013-07-30 at 20:08 -0700, Eric Dumazet wrote:
> > > 
> > > Oh well, this patch is pure noise...
> > > 
> > > 
> > 
> > Mind to be specific?
> > 
> > I know saving 8 bytes is not interesting for you, but it is for me,
> > since I need some room in struct tcp_metrics_block for union inet_addr.
> > With this patch, I don't have to make struct tcp_metrics_block expand to
> > 3 cachelines. :)
> 
> Do you see how this explanation is rather different than the one you
> gave in the changelog ?
> 
> Its 3 lines, instead of all this pahole noise.

So, you mean this patch only makes sense after my union inet_addr? If
so, I will merge it into my inet_addr patch.

> 
> And it would be more logical to put the "unsigned long last_syn_loss" at
> the beginning of the structure, instead after an array of 17 bytes.
> 

Agreed.

Thanks!

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

On Tue, 2013-07-30 at 20:35 -0700, Joe Perches wrote:
> True, using "unsigned long" instead of "long unsigned int"
> would likely be better too. 

"long unsigned int" is the output of pahole, not the source code.

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Eric Dumazet]

On Wed, 2013-07-31 at 11:37 +0800, Cong Wang wrote:

> So, you mean this patch only makes sense after my union inet_addr? If
> so, I will merge it into my inet_addr patch.

What about removing the noise from changelog, and give us the true
story ?

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Wed, 31 Jul 2013 11:13:22 +0800

> I know saving 8 bytes is not interesting for you, but it is for me,
> since I need some room in struct tcp_metrics_block for union inet_addr.
> With this patch, I don't have to make struct tcp_metrics_block expand to
> 3 cachelines. :)

Cong, please.

Instead of making code accomodate an unnecessarily large "union
inet_addr", make a small version of it that accomodates this use case
and doesn't have the extra fields.

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

On Tue, 2013-07-30 at 23:26 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Wed, 31 Jul 2013 11:13:22 +0800
> 
> > I know saving 8 bytes is not interesting for you, but it is for me,
> > since I need some room in struct tcp_metrics_block for union inet_addr.
> > With this patch, I don't have to make struct tcp_metrics_block expand to
> > 3 cachelines. :)
> 
> Cong, please.
> 
> Instead of making code accomodate an unnecessarily large "union
> inet_addr", make a small version of it that accomodates this use case
> and doesn't have the extra fields.

Please teach me how to do that?

The fields not used by inetpeer are sin_port and scope_id etc., and
sin_port is in the middle of sockaddr* structs.

Hmm, maybe just define a new struct by kicking the last field from
struct sockaddr_{in,in6}, so that we can at least shrink 4 bytes.

Except this, I have no idea how to do that.

Thanks!

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Thu, 01 Aug 2013 10:48:31 +0800

> Please teach me how to do that?

struct inet_addr {
	union {
		u32	v4;
		u32	v6[4];
	};
};

ie. exactly what that code is using already.

And it's called called inetpeer_addr, we have it already, there is no
need to make something new.

Making this code use a structure with completely unnecessary and
unused members is pointless and a step backwards.

Re: [Patch net-next] tcp_metrics: rearrange fields to avoid holes

[Cong Wang]

On Thu, 2013-08-01 at 00:15 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Thu, 01 Aug 2013 10:48:31 +0800
> 
> > Please teach me how to do that?
> 
> struct inet_addr {
> 	union {
> 		u32	v4;
> 		u32	v6[4];
> 	};
> };
> 
> ie. exactly what that code is using already.
> 
> And it's called called inetpeer_addr, we have it already, there is no
> need to make something new.
> 
> Making this code use a structure with completely unnecessary and
> unused members is pointless and a step backwards.

Ah, I was thinking to keep it compatible with sockaddr*, actually this
is indeed unnecessary. I am going to define something like:

struct inet_addr_base {
        union {
                struct in_addr  sin_addr;
                struct in6_addr sin6_addr;
        };
        unsigned short int      sin_family;
};

which could used by bridge multicast code too.

Thanks for the hint.