something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

This commit is the result of bisect my problem with window decoration in 
gnome shell ( button, icons, colors etc.)

after revert this commit I don't have any problem with themes windows

a little impossible, commit to the network and affect the graphics might 
be a problem with the allocation and release of memory

my system: 64bit Ubuntu 12.04.5 LTS, kernel next

latest AMD fglrx driver 14.301 ( problem tested and exist in 14.200 also )

Regards,

Krzysztof

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Sun, 2014-10-12 at 19:11 +0200, Krzysztof Kolasa wrote:
> This commit is the result of bisect my problem with window decoration in 
> gnome shell ( button, icons, colors etc.)
> 
> after revert this commit I don't have any problem with themes windows
> 
> a little impossible, commit to the network and affect the graphics might 
> be a problem with the allocation and release of memory
> 
> my system: 64bit Ubuntu 12.04.5 LTS, kernel next
> 
> latest AMD fglrx driver 14.301 ( problem tested and exist in 14.200 also )
> 
> Regards,
> 
> Krzysztof

Hi Krzysztof

This sounds quite strange to me. Some memory corruption might happen
regardless of networking changes, and this commit could uncover an
existing bug.

You might try different SLUB/SLAB choice, and try different debugging
SLUB/SLAB facilities.

With SLUB, skbuff_fclone_cache shares :t-0000512 kmem cache.
You could try slub_nomerge=1 for a start.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

Probably not related with this bug, but with regarding to the
offending commit, what's the point of the memmove() in tcp_v4_rcv()
since ip_rcv() already clears IPCB()?

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <cwang@twopensource.com> wrote:
> Probably not related with this bug, but with regarding to the
> offending commit, what's the point of the memmove() in tcp_v4_rcv()
> since ip_rcv() already clears IPCB()?

Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary
to play with variable-length array with memmove()....

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

W dniu 14.10.2014 o 02:09, Cong Wang pisze:
> On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <cwang@twopensource.com> wrote:
>> Probably not related with this bug, but with regarding to the
>> offending commit, what's the point of the memmove() in tcp_v4_rcv()
>> since ip_rcv() already clears IPCB()?
> Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary
> to play with variable-length array with memmove()....
>
On my other old laptop with 32bit kernel next and graphics card Intel 
945GM just after the revert commit working OK,
before, after login to gnome shell in some seconds decorations disappear

32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014

Can anyone confirm this ?

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Tue, Oct 14, 2014 at 6:25 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
> W dniu 14.10.2014 o 02:09, Cong Wang pisze:
>
>> On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <cwang@twopensource.com> wrote:
>>>
>>> Probably not related with this bug, but with regarding to the
>>> offending commit, what's the point of the memmove() in tcp_v4_rcv()
>>> since ip_rcv() already clears IPCB()?
>>
>> Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary
>> to play with variable-length array with memmove()....
>>
> On my other old laptop with 32bit kernel next and graphics card Intel 945GM
> just after the revert commit working OK,
> before, after login to gnome shell in some seconds decorations disappear
>
> 32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014
>
> Can anyone confirm this ?
>

Sorry, believe it or not, for me it is hard to find a 32bit machine even VM. :)

Could the attached patch by any chance help? I noticed cookie_v4_check()
still uses IPCB() instead of TCPCB() at TCP layer.

Thanks.

[-- Attachment #2: tcp-cb.diff --]
[-- Type: text/plain, Size: 3524 bytes --]

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 74efeda..5d11012 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -468,8 +468,7 @@ void inet_sk_rx_dst_set(struct sock *sk, const struct sk_buff *skb);
 /* From syncookies.c */
 int __cookie_v4_check(const struct iphdr *iph, const struct tcphdr *th,
 		      u32 cookie);
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
-			     struct ip_options *opt);
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb);
 #ifdef CONFIG_SYN_COOKIES
 
 /* Syncookies use a monotonic timer which increments every 60 seconds.
@@ -530,6 +529,7 @@ void tcp_send_active_reset(struct sock *sk, gfp_t priority);
 int tcp_send_synack(struct sock *);
 bool tcp_syn_flood_action(struct sock *sk, const struct sk_buff *skb,
 			  const char *proto);
+struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb);
 void tcp_push_one(struct sock *, unsigned int mss_now);
 void tcp_send_ack(struct sock *sk);
 void tcp_send_delayed_ack(struct sock *sk);
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 0431a8f..d346303 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -255,9 +255,9 @@ bool cookie_check_timestamp(struct tcp_options_received *tcp_opt,
 }
 EXPORT_SYMBOL(cookie_check_timestamp);
 
-struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
-			     struct ip_options *opt)
+struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
 {
+	struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	struct tcp_options_received tcp_opt;
 	struct inet_request_sock *ireq;
 	struct tcp_request_sock *treq;
@@ -317,15 +317,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	/* We throwed the options of the initial SYN away, so we hope
 	 * the ACK carries the same options again (see RFC1122 4.2.3.8)
 	 */
-	if (opt && opt->optlen) {
-		int opt_size = sizeof(struct ip_options_rcu) + opt->optlen;
-
-		ireq->opt = kmalloc(opt_size, GFP_ATOMIC);
-		if (ireq->opt != NULL && ip_options_echo(&ireq->opt->opt, skb)) {
-			kfree(ireq->opt);
-			ireq->opt = NULL;
-		}
-	}
+	ireq->opt = tcp_v4_save_options(skb);
 
 	if (security_inet_conn_request(sk, skb, req)) {
 		reqsk_free(req);
@@ -344,7 +336,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	flowi4_init_output(&fl4, sk->sk_bound_dev_if, ireq->ir_mark,
 			   RT_CONN_FLAGS(sk), RT_SCOPE_UNIVERSE, IPPROTO_TCP,
 			   inet_sk_flowi_flags(sk),
-			   (opt && opt->srr) ? opt->faddr : ireq->ir_rmt_addr,
+			   opt->srr ? opt->faddr : ireq->ir_rmt_addr,
 			   ireq->ir_loc_addr, th->source, th->dest);
 	security_req_classify_flow(req, flowi4_to_flowi(&fl4));
 	rt = ip_route_output_key(sock_net(sk), &fl4);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 552e87e..e02d586 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -883,7 +883,7 @@ EXPORT_SYMBOL(tcp_syn_flood_action);
 /*
  * Save and compile IPv4 options into the request_sock if needed.
  */
-static struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb)
+struct ip_options_rcu *tcp_v4_save_options(struct sk_buff *skb)
 {
 	const struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
 	struct ip_options_rcu *dopt = NULL;
@@ -1428,7 +1428,7 @@ static struct sock *tcp_v4_hnd_req(struct sock *sk, struct sk_buff *skb)
 
 #ifdef CONFIG_SYN_COOKIES
 	if (!th->syn)
-		sk = cookie_v4_check(sk, skb, &TCP_SKB_CB(skb)->header.h4.opt);
+		sk = cookie_v4_check(sk, skb);
 #endif
 	return sk;
 }

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[David Miller]

From: Cong Wang <cwang@twopensource.com>
Date: Tue, 14 Oct 2014 11:59:25 -0700

> On Tue, Oct 14, 2014 at 6:25 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
>> W dniu 14.10.2014 o 02:09, Cong Wang pisze:
>>
>>> On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <cwang@twopensource.com> wrote:
>>>>
>>>> Probably not related with this bug, but with regarding to the
>>>> offending commit, what's the point of the memmove() in tcp_v4_rcv()
>>>> since ip_rcv() already clears IPCB()?
>>>
>>> Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks scary
>>> to play with variable-length array with memmove()....
>>>
>> On my other old laptop with 32bit kernel next and graphics card Intel 945GM
>> just after the revert commit working OK,
>> before, after login to gnome shell in some seconds decorations disappear
>>
>> 32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014
>>
>> Can anyone confirm this ?
>>
> 
> Sorry, believe it or not, for me it is hard to find a 32bit machine even VM. :)
> 
> Could the attached patch by any chance help? I noticed cookie_v4_check()
> still uses IPCB() instead of TCPCB() at TCP layer.

Eric, please review.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[David Miller]

From: Eric Dumazet <edumazet@gmail.com>
Date: Tue, 14 Oct 2014 13:42:01 -0700

> 2014-10-14 12:03 GMT-07:00 David Miller <davem@davemloft.net>:
> 
>> From: Cong Wang <cwang@twopensource.com>
>> Date: Tue, 14 Oct 2014 11:59:25 -0700
>>
>> > On Tue, Oct 14, 2014 at 6:25 AM, Krzysztof Kolasa <kkolasa@winsoft.pl>
>> wrote:
>> >> W dniu 14.10.2014 o 02:09, Cong Wang pisze:
>> >>
>> >>> On Mon, Oct 13, 2014 at 4:59 PM, Cong Wang <cwang@twopensource.com>
>> wrote:
>> >>>>
>> >>>> Probably not related with this bug, but with regarding to the
>> >>>> offending commit, what's the point of the memmove() in tcp_v4_rcv()
>> >>>> since ip_rcv() already clears IPCB()?
>> >>>
>> >>> Oh, ip options are actually saved in ip_rcv_finish()... Hmm, looks
>> scary
>> >>> to play with variable-length array with memmove()....
>> >>>
>> >> On my other old laptop with 32bit kernel next and graphics card Intel
>> 945GM
>> >> just after the revert commit working OK,
>> >> before, after login to gnome shell in some seconds decorations disappear
>> >>
>> >> 32 bit Ubuntu 12.04.5 LTS, gnome shell, kernel source next 14-10-2014
>> >>
>> >> Can anyone confirm this ?
>> >>
>> >
>> > Sorry, believe it or not, for me it is hard to find a 32bit machine even
>> VM. :)
>> >
>> > Could the attached patch by any chance help? I noticed cookie_v4_check()
>> > still uses IPCB() instead of TCPCB() at TCP layer.
>>
>> Eric, please review.
>>
> 
> For an unknown reason I do not see this patch on my eric.dumazet@gmail.com
> 
> It seems my migration to Trusty was not a good move :(
> 
> Patch looks good, but seems a net-next candidate to me.

I thought it is a potential fix for this corruption report?

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

2014-10-14 13:47 GMT-07:00 David Miller <davem@davemloft.net>:
>
>
> > Patch looks good, but seems a net-next candidate to me.
>
> I thought it is a potential fix for this corruption report?


Sure, but a one liner fix seemed more obvious, as it points to the problem.

I do not particularly care, as long as we fixed the bug.

Thanks !

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[David Miller]

From: Eric Dumazet <edumazet@gmail.com>
Date: Tue, 14 Oct 2014 14:03:51 -0700

> 2014-10-14 13:47 GMT-07:00 David Miller <davem@davemloft.net>:
>>
>>
>> > Patch looks good, but seems a net-next candidate to me.
>>
>> I thought it is a potential fix for this corruption report?
> 
> 
> Sure, but a one liner fix seemed more obvious, as it points to the problem.
> 
> I do not particularly care, as long as we fixed the bug.

Ok, let's get testing feedback and then I'll look for a formal submission.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

On Tue, Oct 14, 2014 at 2:15 PM, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <edumazet@gmail.com>
> Date: Tue, 14 Oct 2014 14:03:51 -0700
>
>> 2014-10-14 13:47 GMT-07:00 David Miller <davem@davemloft.net>:
>>>
>>>
>>> > Patch looks good, but seems a net-next candidate to me.
>>>
>>> I thought it is a potential fix for this corruption report?
>>
>>
>> Sure, but a one liner fix seemed more obvious, as it points to the problem.
>>
>> I do not particularly care, as long as we fixed the bug.


Since we are still in merge window, I don't think we have to use
a one-line fix for a bug introduced in this merge window.

>
> Ok, let's get testing feedback and then I'll look for a formal submission.

Yeah, there is no evidence to say my patch fixes the bug Krzysztof
reported before he tests it, I found it during code review.

Thanks!

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Tue, 2014-10-14 at 14:24 -0700, Cong Wang wrote:

> 
> Since we are still in merge window, I don't think we have to use
> a one-line fix for a bug introduced in this merge window.

You are clearly refactoring here. Its a nice cleanup.

If I was the maintainer, I would prefer the one line fix.

Then when net-next is open, you refactor.

As I said, I wont argue, do whatever you want.

Thanks

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:
> On Tue, 2014-10-14 at 14:24 -0700, Cong Wang wrote:
>
>> Since we are still in merge window, I don't think we have to use
>> a one-line fix for a bug introduced in this merge window.
> You are clearly refactoring here. Its a nice cleanup.
>
> If I was the maintainer, I would prefer the one line fix.
>
> Then when net-next is open, you refactor.
>
> As I said, I wont argue, do whatever you want.
>
> Thanks
>
>
>

one-line patch not resolve problem, fix created by Cong Wang resolves 
problem !!!

tested on 64bit system and working OK

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Wed, 2014-10-15 at 12:35 +0200, Krzysztof Kolasa wrote:
> W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:
> > On Tue, 2014-10-14 at 14:24 -0700, Cong Wang wrote:
> >
> >> Since we are still in merge window, I don't think we have to use
> >> a one-line fix for a bug introduced in this merge window.
> > You are clearly refactoring here. Its a nice cleanup.
> >
> > If I was the maintainer, I would prefer the one line fix.
> >
> > Then when net-next is open, you refactor.
> >
> > As I said, I wont argue, do whatever you want.
> >
> > Thanks
> >
> >
> >
> 
> one-line patch not resolve problem, fix created by Cong Wang resolves 
> problem !!!

Hmm, there should be no difference with either patch.

tcp_v4_rcv() 
...
memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
        sizeof(struct inet_skb_parm));
...
-> tcp_v4_do_rcv() 
   ->  tcp_v4_hnd_req()
      -> cookie_v4_check(... , &TCP_SKB_CB(skb)->header.h4.opt)

Hmm...

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

W dniu 15.10.2014 o 13:27, Eric Dumazet pisze:
> On Wed, 2014-10-15 at 12:35 +0200, Krzysztof Kolasa wrote:
>> W dniu 14.10.2014 o 23:43, Eric Dumazet pisze:
>>> On Tue, 2014-10-14 at 14:24 -0700, Cong Wang wrote:
>>>
>>>> Since we are still in merge window, I don't think we have to use
>>>> a one-line fix for a bug introduced in this merge window.
>>> You are clearly refactoring here. Its a nice cleanup.
>>>
>>> If I was the maintainer, I would prefer the one line fix.
>>>
>>> Then when net-next is open, you refactor.
>>>
>>> As I said, I wont argue, do whatever you want.
>>>
>>> Thanks
>>>
>>>
>>>
>> one-line patch not resolve problem, fix created by Cong Wang resolves
>> problem !!!
> Hmm, there should be no difference with either patch.
>
> tcp_v4_rcv()
> ...
> memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
>          sizeof(struct inet_skb_parm));
> ...
> -> tcp_v4_do_rcv()
>     ->  tcp_v4_hnd_req()
>        -> cookie_v4_check(... , &TCP_SKB_CB(skb)->header.h4.opt)
>
> Hmm...
>
>

on a 32bit system, the patch did not solve the problem :(
I have exactly the same problem as before the patch

I do not understand this, perhaps the problem is hidden somewhere else,
one thing is certain after revert commit 971f10eca1 everything works 
correctly

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Wed, 2014-10-15 at 13:40 +0200, Krzysztof Kolasa wrote:

> on a 32bit system, the patch did not solve the problem :(
> I have exactly the same problem as before the patch


You mean Cong patch ?

> 
> I do not understand this, perhaps the problem is hidden somewhere else,
> one thing is certain after revert commit 971f10eca1 everything works 
> correctly
> 

Can you privately send me the vmlinux file ?

It might be a compiler issue...

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

W dniu 15.10.2014 o 17:32, Eric Dumazet pisze:
> On Wed, 2014-10-15 at 13:40 +0200, Krzysztof Kolasa wrote:
>
>> on a 32bit system, the patch did not solve the problem :(
>> I have exactly the same problem as before the patch
>
> You mean Cong patch ?
Yes
>
>> I do not understand this, perhaps the problem is hidden somewhere else,
>> one thing is certain after revert commit 971f10eca1 everything works
>> correctly
>>
> Can you privately send me the vmlinux file ?
>
> It might be a compiler issue...
>
>
>
I sent the file

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
>
> one-line patch not resolve problem, fix created by Cong Wang resolves
> problem !!!
>
> tested on 64bit system and working OK
>

So my patch fixes your problem on 64bit but not on 32bit,
is this correct?

I will send out the patch. And as Eric said, the problem on 32bit
might be a different issue, we can definitely fix it separately.

Thanks for testing!

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Krzysztof Kolasa]

W dniu 15.10.2014 o 22:36, Cong Wang pisze:
> On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
>> one-line patch not resolve problem, fix created by Cong Wang resolves
>> problem !!!
>>
>> tested on 64bit system and working OK
>>
> So my patch fixes your problem on 64bit but not on 32bit,
> is this correct?
Yes

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

On Wed, Oct 15, 2014 at 1:42 PM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
> W dniu 15.10.2014 o 22:36, Cong Wang pisze:
>>
>> On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl>
>> wrote:
>>>
>>> one-line patch not resolve problem, fix created by Cong Wang resolves
>>> problem !!!
>>>
>>> tested on 64bit system and working OK
>>>
>> So my patch fixes your problem on 64bit but not on 32bit,
>> is this correct?
>
> Yes

Cool! Patches are coming.

Meanwhile Eric is debugging it, do you mind to try a followup quick
fix on your 32bit system?

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e13d778..12bd3f6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1006,8 +1006,7 @@ static int tcp_transmit_skb(struct sock *sk,
struct sk_buff *skb, int clone_it,
        skb->tstamp.tv64 = 0;

        /* Cleanup our debris for IP stacks */
-       memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
-                              sizeof(struct inet6_skb_parm)));
+       memset(TCPCB(skb), 0, sizeof(*TCPCB(skb)));

        err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Cong Wang]

On Wed, Oct 15, 2014 at 2:22 PM, Cong Wang <cwang@twopensource.com> wrote:
>
> Meanwhile Eric is debugging it, do you mind to try a followup quick
> fix on your 32bit system?
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e13d778..12bd3f6 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1006,8 +1006,7 @@ static int tcp_transmit_skb(struct sock *sk,
> struct sk_buff *skb, int clone_it,
>         skb->tstamp.tv64 = 0;
>
>         /* Cleanup our debris for IP stacks */
> -       memset(skb->cb, 0, max(sizeof(struct inet_skb_parm),
> -                              sizeof(struct inet6_skb_parm)));
> +       memset(TCPCB(skb), 0, sizeof(*TCPCB(skb)));


Of course, s/TCPCB/TCP_SKB_CB/. :)

>
>         err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl);

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Wed, 2014-10-15 at 13:36 -0700, Cong Wang wrote:
> On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
> >
> > one-line patch not resolve problem, fix created by Cong Wang resolves
> > problem !!!
> >
> > tested on 64bit system and working OK
> >
> 
> So my patch fixes your problem on 64bit but not on 32bit,
> is this correct?
> 
> I will send out the patch. And as Eric said, the problem on 32bit
> might be a different issue, we can definitely fix it separately.
> 
> Thanks for testing!

I think more fixes are needed.

This is most probably an IPv6 issue.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Wed, 2014-10-15 at 14:39 -0700, Eric Dumazet wrote:
> On Wed, 2014-10-15 at 13:36 -0700, Cong Wang wrote:
> > On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
> > >
> > > one-line patch not resolve problem, fix created by Cong Wang resolves
> > > problem !!!
> > >
> > > tested on 64bit system and working OK
> > >
> > 
> > So my patch fixes your problem on 64bit but not on 32bit,
> > is this correct?
> > 
> > I will send out the patch. And as Eric said, the problem on 32bit
> > might be a different issue, we can definitely fix it separately.
> > 
> > Thanks for testing!
> 
> I think more fixes are needed.
> 
> This is most probably an IPv6 issue.
> 

Yes, ipv6 uses inet6_iif() a lot. We need to use a different accessor.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 15 Oct 2014 14:42:34 -0700

> On Wed, 2014-10-15 at 14:39 -0700, Eric Dumazet wrote:
>> On Wed, 2014-10-15 at 13:36 -0700, Cong Wang wrote:
>> > On Wed, Oct 15, 2014 at 3:35 AM, Krzysztof Kolasa <kkolasa@winsoft.pl> wrote:
>> > >
>> > > one-line patch not resolve problem, fix created by Cong Wang resolves
>> > > problem !!!
>> > >
>> > > tested on 64bit system and working OK
>> > >
>> > 
>> > So my patch fixes your problem on 64bit but not on 32bit,
>> > is this correct?
>> > 
>> > I will send out the patch. And as Eric said, the problem on 32bit
>> > might be a different issue, we can definitely fix it separately.
>> > 
>> > Thanks for testing!
>> 
>> I think more fixes are needed.
>> 
>> This is most probably an IPv6 issue.
>> 
> 
> Yes, ipv6 uses inet6_iif() a lot. We need to use a different accessor.

So I applied Cong's patches, but if I am to understand the testing feedback
so far only the 64-bit case of the reporter's problems is fixed, and that
the 32-bit graphics corruption still happens.

Can I get some clarification of where we stand after Cong's fixes
which are already merged into 'net'?

Thanks.

Re: something is wrong in commit 971f10eca1 - tcp: better TCP_SKB_CB layout to reduce cache line misses

[Eric Dumazet]

On Fri, 2014-10-17 at 15:29 -0400, David Miller wrote:
> So I applied Cong's patches, but if I am to understand the testing feedback
> so far only the 64-bit case of the reporter's problems is fixed, and that
> the 32-bit graphics corruption still happens.
> 
> Can I get some clarification of where we stand after Cong's fixes
> which are already merged into 'net'?

I spent some time on the (awful) memmove() implementation on x86_32, and
could not find a bug in it.

It would be worth trying doing 2 memcpy() instead just in case...