| Message ID | 1375238905-6423-1-git-send-email-amwang@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {