Kernel panic eth2 mirred redirect to ifb0

Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

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

Hi all

I have panic with kernel 2.6.37-rc6-git2 when use iproute2 
mirred/redirect action


host1 (kernel 2.6.36.2)
netperf client -> eth3 (82598EB 10-Gigabit AT CX4) - directly connected 
to eth2 of host2
ethtool -k eth3
Offload parameters for eth3:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off
rx-vlan-offload: off
tx-vlan-offload: off
ntuple-filters: off
receive-hashing: off

ethtool -i eth3
driver: ixgbe
version: 2.0.84-k2
firmware-version: 1.12-2
bus-info: 0000:03:00.1


host2 (kernel-2.6.37-rc6-git2)
netserver -> eth2 (82598EB 10-Gigabit AT CX4) - directly connected to 
eth3 of host1

ethtool -k eth2
Offload parameters for eth2:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off
rx-vlan-offload: on
tx-vlan-offload: on
ntuple-filters: off
receive-hashing: off

ethtool -i eth2
driver: ixgbe
version: 2.0.84-k2
firmware-version: 1.12-2
bus-info: 0000:03:00.0



Normally without ifb and redirect netperf show:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.0.2 
(192.168.0.2) port 0 AF_INET
Recv   Send    Send
Socket Socket  Message  Elapsed
Size   Size    Size     Time     Throughput
bytes  bytes   bytes    secs.    10^6bits/sec

  87380  16384  16384    10.00    9042.14


Steps to reproduce panic:
ip link set dev ifb0 up

tc qdisc add dev eth2 ingress

tc filter add dev eth2 parent ffff: protocol ip prio 10 u32 \
match ip src 0.0.0.0/0 flowid 1:1 \
action mirred egress redirect dev ifb0


After this when i start netperf on host1 I have panic (screenshot in 
attached image).

Thanks
Pawel

[-- Attachment #2: ifb-eth2-mirred-redirect-panic.JPG --]
[-- Type: image/jpeg, Size: 105170 bytes --]

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

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

W dniu 2010-12-19 12:35, Paweł Staszewski pisze:
> Hi all
>
> I have panic with kernel 2.6.37-rc6-git2 when use iproute2 
> mirred/redirect action
>
>
> host1 (kernel 2.6.36.2)
> netperf client -> eth3 (82598EB 10-Gigabit AT CX4) - directly 
> connected to eth2 of host2
> ethtool -k eth3
> Offload parameters for eth3:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
> rx-vlan-offload: off
> tx-vlan-offload: off
> ntuple-filters: off
> receive-hashing: off
>
> ethtool -i eth3
> driver: ixgbe
> version: 2.0.84-k2
> firmware-version: 1.12-2
> bus-info: 0000:03:00.1
>
>
> host2 (kernel-2.6.37-rc6-git2)
> netserver -> eth2 (82598EB 10-Gigabit AT CX4) - directly connected to 
> eth3 of host1
>
> ethtool -k eth2
> Offload parameters for eth2:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
> rx-vlan-offload: on
> tx-vlan-offload: on
> ntuple-filters: off
> receive-hashing: off
>
> ethtool -i eth2
> driver: ixgbe
> version: 2.0.84-k2
> firmware-version: 1.12-2
> bus-info: 0000:03:00.0
>
>
>
> Normally without ifb and redirect netperf show:
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.0.2 
> (192.168.0.2) port 0 AF_INET
> Recv   Send    Send
> Socket Socket  Message  Elapsed
> Size   Size    Size     Time     Throughput
> bytes  bytes   bytes    secs.    10^6bits/sec
>
>  87380  16384  16384    10.00    9042.14
>
>
> Steps to reproduce panic:
> ip link set dev ifb0 up
>
> tc qdisc add dev eth2 ingress
>
> tc filter add dev eth2 parent ffff: protocol ip prio 10 u32 \
> match ip src 0.0.0.0/0 flowid 1:1 \
> action mirred egress redirect dev ifb0
>
>
> After this when i start netperf on host1 I have panic (screenshot in 
> attached image).
>
> Thanks
> Pawel
In next attached image is second screenshot (different - trace) - but 
with the same configuration


Regards
Pawel

[-- Attachment #2: ifb-eth2-mirred-redirect-panic-part2.JPG --]
[-- Type: image/jpeg, Size: 91665 bytes --]

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le dimanche 19 décembre 2010 à 12:35 +0100, Paweł Staszewski a écrit :
> Hi all
> 
> I have panic with kernel 2.6.37-rc6-git2 when use iproute2 
> mirred/redirect action
> 
> 
> host1 (kernel 2.6.36.2)
> netperf client -> eth3 (82598EB 10-Gigabit AT CX4) - directly connected 
> to eth2 of host2
> ethtool -k eth3
> Offload parameters for eth3:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
> rx-vlan-offload: off
> tx-vlan-offload: off
> ntuple-filters: off
> receive-hashing: off
> 
> ethtool -i eth3
> driver: ixgbe
> version: 2.0.84-k2
> firmware-version: 1.12-2
> bus-info: 0000:03:00.1
> 
> 
> host2 (kernel-2.6.37-rc6-git2)
> netserver -> eth2 (82598EB 10-Gigabit AT CX4) - directly connected to 
> eth3 of host1
> 
> ethtool -k eth2
> Offload parameters for eth2:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
> rx-vlan-offload: on
> tx-vlan-offload: on
> ntuple-filters: off
> receive-hashing: off
> 
> ethtool -i eth2
> driver: ixgbe
> version: 2.0.84-k2
> firmware-version: 1.12-2
> bus-info: 0000:03:00.0
> 
> 
> 
> Normally without ifb and redirect netperf show:
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.0.2 
> (192.168.0.2) port 0 AF_INET
> Recv   Send    Send
> Socket Socket  Message  Elapsed
> Size   Size    Size     Time     Throughput
> bytes  bytes   bytes    secs.    10^6bits/sec
> 
>   87380  16384  16384    10.00    9042.14
> 
> 
> Steps to reproduce panic:
> ip link set dev ifb0 up
> 
> tc qdisc add dev eth2 ingress
> 
> tc filter add dev eth2 parent ffff: protocol ip prio 10 u32 \
> match ip src 0.0.0.0/0 flowid 1:1 \
> action mirred egress redirect dev ifb0
> 
> 
> After this when i start netperf on host1 I have panic (screenshot in 
> attached image).

Unfortunately, we miss the start of panic messages. Could you try to get
them ?

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

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

W dniu 2010-12-19 16:43, Eric Dumazet pisze:
> Le dimanche 19 décembre 2010 à 12:35 +0100, Paweł Staszewski a écrit :
>> Hi all
>>
>> I have panic with kernel 2.6.37-rc6-git2 when use iproute2
>> mirred/redirect action
>>
>>
>> host1 (kernel 2.6.36.2)
>> netperf client ->  eth3 (82598EB 10-Gigabit AT CX4) - directly connected
>> to eth2 of host2
>> ethtool -k eth3
>> Offload parameters for eth3:
>> rx-checksumming: on
>> tx-checksumming: on
>> scatter-gather: on
>> tcp-segmentation-offload: on
>> udp-fragmentation-offload: off
>> generic-segmentation-offload: on
>> generic-receive-offload: on
>> large-receive-offload: off
>> rx-vlan-offload: off
>> tx-vlan-offload: off
>> ntuple-filters: off
>> receive-hashing: off
>>
>> ethtool -i eth3
>> driver: ixgbe
>> version: 2.0.84-k2
>> firmware-version: 1.12-2
>> bus-info: 0000:03:00.1
>>
>>
>> host2 (kernel-2.6.37-rc6-git2)
>> netserver ->  eth2 (82598EB 10-Gigabit AT CX4) - directly connected to
>> eth3 of host1
>>
>> ethtool -k eth2
>> Offload parameters for eth2:
>> rx-checksumming: on
>> tx-checksumming: on
>> scatter-gather: on
>> tcp-segmentation-offload: on
>> udp-fragmentation-offload: off
>> generic-segmentation-offload: on
>> generic-receive-offload: on
>> large-receive-offload: off
>> rx-vlan-offload: on
>> tx-vlan-offload: on
>> ntuple-filters: off
>> receive-hashing: off
>>
>> ethtool -i eth2
>> driver: ixgbe
>> version: 2.0.84-k2
>> firmware-version: 1.12-2
>> bus-info: 0000:03:00.0
>>
>>
>>
>> Normally without ifb and redirect netperf show:
>> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.0.2
>> (192.168.0.2) port 0 AF_INET
>> Recv   Send    Send
>> Socket Socket  Message  Elapsed
>> Size   Size    Size     Time     Throughput
>> bytes  bytes   bytes    secs.    10^6bits/sec
>>
>>    87380  16384  16384    10.00    9042.14
>>
>>
>> Steps to reproduce panic:
>> ip link set dev ifb0 up
>>
>> tc qdisc add dev eth2 ingress
>>
>> tc filter add dev eth2 parent ffff: protocol ip prio 10 u32 \
>> match ip src 0.0.0.0/0 flowid 1:1 \
>> action mirred egress redirect dev ifb0
>>
>>
>> After this when i start netperf on host1 I have panic (screenshot in
>> attached image).
> Unfortunately, we miss the start of panic messages. Could you try to get
> them ?
>
In attached images

Regards
Pawel

>
> --
> 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
>
>


[-- Attachment #2: kpanic-part1.JPG --]
[-- Type: image/jpeg, Size: 107573 bytes --]

[-- Attachment #3: kpanic-part2.JPG --]
[-- Type: image/jpeg, Size: 90615 bytes --]

Re: Kernel panic eth2 mirred redirect to ifb0

[Changli Gao]

2010/12/20 Paweł Staszewski <pstaszewski@itcare.pl>:
> W dniu 2010-12-19 16:43, Eric Dumazet pisze:
>>
>> Unfortunately, we miss the start of panic messages. Could you try to get
>> them ?
>>
> In attached images
>

It seems the kernel panic at:

        if (skb_shared(skb))
                BUG();

in pskb_expand_head().

It maybe related to my patch:
http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=210d6de78c5d7c785fc532556cea340e517955e1

You can try to revert it and test again.

However, the bug is a misuse of pskb_expand_head().

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

W dniu 2010-12-19 17:22, Changli Gao pisze:
> 2010/12/20 Paweł Staszewski<pstaszewski@itcare.pl>:
>> W dniu 2010-12-19 16:43, Eric Dumazet pisze:
>>> Unfortunately, we miss the start of panic messages. Could you try to get
>>> them ?
>>>
>> In attached images
>>
> It seems the kernel panic at:
>
>          if (skb_shared(skb))
>                  BUG();
>
> in pskb_expand_head().
>
> It maybe related to my patch:
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=210d6de78c5d7c785fc532556cea340e517955e1
>
> You can try to revert it and test again.
>
> However, the bug is a misuse of pskb_expand_head().
>
patching file net/sched/act_mirred.c
Hunk #1 FAILED at 169.
Hunk #2 succeeded at 195 (offset 10 lines).
1 out of 2 hunks FAILED -- saving rejects to file net/sched/act_mirred.c.rej

***************
*** 169,181 ****
                 goto out;
         }

-       at = G_TC_AT(skb->tc_verd);
-       skb2 = skb_act_clone(skb, GFP_ATOMIC, m->tcf_action);
         if (skb2 == NULL)
                 goto out;

         m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
         m->tcf_bstats.packets++;
         if (!(at & AT_EGRESS)) {
                 if (m->tcfm_ok_push)
                         skb_push(skb2, skb2->dev->hard_header_len);
--- 169,181 ----
                 goto out;
         }

+       skb2 = skb_act_clone(skb, GFP_ATOMIC);
         if (skb2 == NULL)
                 goto out;

         m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
         m->tcf_bstats.packets++;
+       at = G_TC_AT(skb->tc_verd);
         if (!(at & AT_EGRESS)) {
                 if (m->tcfm_ok_push)
                         skb_push(skb2, skb2->dev->hard_header_len);

for sch_generic.h was ok.

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

Paweł Staszewski wrote:
> W dniu 2010-12-19 17:22, Changli Gao pisze:
>> 2010/12/20 Paweł Staszewski<pstaszewski@itcare.pl>:
>>> W dniu 2010-12-19 16:43, Eric Dumazet pisze:
>>>> Unfortunately, we miss the start of panic messages. Could you try to
>>>> get
>>>> them ?
>>>>
>>> In attached images
>>>
>> It seems the kernel panic at:
>>
>>          if (skb_shared(skb))
>>                  BUG();
>>
>> in pskb_expand_head().
>>
>> It maybe related to my patch:
>> http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=210d6de78c5d7c785fc532556cea340e517955e1
>>
>>
>> You can try to revert it and test again.
>>
>> However, the bug is a misuse of pskb_expand_head().
>>
> patching file net/sched/act_mirred.c
> Hunk #1 FAILED at 169.
> Hunk #2 succeeded at 195 (offset 10 lines).
> 1 out of 2 hunks FAILED -- saving rejects to file
> net/sched/act_mirred.c.rej
> 
> ***************
> *** 169,181 ****
>                 goto out;
>         }
> 
> -       at = G_TC_AT(skb->tc_verd);
> -       skb2 = skb_act_clone(skb, GFP_ATOMIC, m->tcf_action);
>         if (skb2 == NULL)
>                 goto out;
> 
>         m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
>         m->tcf_bstats.packets++;
>         if (!(at & AT_EGRESS)) {
>                 if (m->tcfm_ok_push)
>                         skb_push(skb2, skb2->dev->hard_header_len);
> --- 169,181 ----
>                 goto out;
>         }
> 
> +       skb2 = skb_act_clone(skb, GFP_ATOMIC);
>         if (skb2 == NULL)
>                 goto out;
> 
>         m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
>         m->tcf_bstats.packets++;
> +       at = G_TC_AT(skb->tc_verd);
>         if (!(at & AT_EGRESS)) {
>                 if (m->tcfm_ok_push)
>                         skb_push(skb2, skb2->dev->hard_header_len);
> 
> for sch_generic.h was ok.

Should be enough to try after reverting this sch_generic.h change only.

Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

Jarek Poplawski wrote:
> Paweł Staszewski wrote:

>> for sch_generic.h was ok.
> 
> Should be enough to try after reverting this sch_generic.h change only.

Hmm... Sorry, I meant the change inside skb_act_clone(). I'll send a patch.
 
Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

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

Jarek Poplawski wrote:
> Jarek Poplawski wrote:
>> Paweł Staszewski wrote:
> 
>>> for sch_generic.h was ok.
>>
>> Should be enough to try after reverting this sch_generic.h change only.
> 
> Hmm... Sorry, I meant the change inside skb_act_clone(). I'll send a patch.

Here it is.
  
Jarek P.


[-- Attachment #2: sch_generic.h.act_clone.1.diff --]
[-- Type: text/plain, Size: 644 bytes --]

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index ea1f8a8..8763ccc 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -608,13 +608,7 @@ static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
 static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask,
 					    int action)
 {
-	struct sk_buff *n;
-
-	if ((action == TC_ACT_STOLEN || action == TC_ACT_QUEUED) &&
-	    !skb_shared(skb))
-		n = skb_get(skb);
-	else
-		n = skb_clone(skb, gfp_mask);
+	struct sk_buff *n = skb_clone(skb, gfp_mask);
 
 	if (n) {
 		n->tc_verd = SET_TC_VERD(n->tc_verd, 0);

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

W dniu 2010-12-19 23:26, Jarek Poplawski pisze:
> Jarek Poplawski wrote:
>> Jarek Poplawski wrote:
>>> Paweł Staszewski wrote:
>>>> for sch_generic.h was ok.
>>> Should be enough to try after reverting this sch_generic.h change only.
>> Hmm... Sorry, I meant the change inside skb_act_clone(). I'll send a patch.
> Here it is.
>
> Jarek P.
>
Yes reverting this patch solves the problem.


Thanks
Paweł

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 09:01 +0100, Paweł Staszewski a écrit :
> W dniu 2010-12-19 23:26, Jarek Poplawski pisze:
> > Jarek Poplawski wrote:
> >> Jarek Poplawski wrote:
> >>> Paweł Staszewski wrote:
> >>>> for sch_generic.h was ok.
> >>> Should be enough to try after reverting this sch_generic.h change only.
> >> Hmm... Sorry, I meant the change inside skb_act_clone(). I'll send a patch.
> > Here it is.
> >
> > Jarek P.
> >
> Yes reverting this patch solves the problem.

Thanks !

I tried to reproduce the crash without success. Might be related to 10Gb
speed. (I only have 1Gb here right now)

Re: Kernel panic eth2 mirred redirect to ifb0

[Changli Gao]

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

2010/12/20 Paweł Staszewski <pstaszewski@itcare.pl>:
>>
> Yes reverting this patch solves the problem.
>
>

I am not sure reverting is the right fix. Maybe you can try this patch
attached instead.

BTW: there are some others NIC drivers and net_sched actions don't
take care of shared skbs.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 578 bytes --]

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ca9036d..602cd32 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6096,6 +6096,15 @@ static int ixgbe_tso(struct ixgbe_adapter *adapter,
 	u32 mss_l4len_idx, l4len;
 
 	if (skb_is_gso(skb)) {
+		if (skb_shared(skb)) {
+			struct sk_buff *nskb;
+
+			nskb = skb_clone(skb, GFP_ATOMIC);
+			if (!nskb)
+				return -ENOMEM;
+			kfree_skb(skb);
+			skb = nskb;
+		}
 		if (skb_header_cloned(skb)) {
 			err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
 			if (err)

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit :
> 2010/12/20 Paweł Staszewski <pstaszewski@itcare.pl>:
> >>
> > Yes reverting this patch solves the problem.
> >
> >
> 
> I am not sure reverting is the right fix. Maybe you can try this patch
> attached instead.
> 
> BTW: there are some others NIC drivers and net_sched actions don't
> take care of shared skbs.
> 

This patch cant be right as is.

        if (skb_is_gso(skb)) {
+               if (skb_shared(skb)) {
+                       struct sk_buff *nskb;
+
+                       nskb = skb_clone(skb, GFP_ATOMIC);
+                       if (!nskb)
+                               return -ENOMEM;
+                       kfree_skb(skb); // HERE
+                       skb = nskb;
+               }

You free original skb, while caller might dereference it later.
So you must pass cloned skb back to caller.

(line 6393 :)
 count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first);

Re: Kernel panic eth2 mirred redirect to ifb0

[Changli Gao]

On Mon, Dec 20, 2010 at 5:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit :
>
> This patch cant be right as is.
>
>        if (skb_is_gso(skb)) {
> +               if (skb_shared(skb)) {
> +                       struct sk_buff *nskb;
> +
> +                       nskb = skb_clone(skb, GFP_ATOMIC);
> +                       if (!nskb)
> +                               return -ENOMEM;
> +                       kfree_skb(skb); // HERE
> +                       skb = nskb;
> +               }
>
> You free original skb, while caller might dereference it later.
> So you must pass cloned skb back to caller.
>
> (line 6393 :)
>  count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first);
>
>

Yes, you are right. I just wonder where shared skbs are allowed. Is
there any rule?

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

W dniu 2010-12-20 09:06, Eric Dumazet pisze:
> Le lundi 20 décembre 2010 à 09:01 +0100, Paweł Staszewski a écrit :
>> W dniu 2010-12-19 23:26, Jarek Poplawski pisze:
>>> Jarek Poplawski wrote:
>>>> Jarek Poplawski wrote:
>>>>> Paweł Staszewski wrote:
>>>>>> for sch_generic.h was ok.
>>>>> Should be enough to try after reverting this sch_generic.h change only.
>>>> Hmm... Sorry, I meant the change inside skb_act_clone(). I'll send a patch.
>>> Here it is.
>>>
>>> Jarek P.
>>>
>> Yes reverting this patch solves the problem.
> Thanks !
>
> I tried to reproduce the crash without success. Might be related to 10Gb
> speed. (I only have 1Gb here right now)
>
>
Yes this is only ixgbe 10Gbit issue on e1000e all is working correctly.

> --
> 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
>
>

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 17:11 +0800, Changli Gao a écrit :
> On Mon, Dec 20, 2010 at 5:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit :
> >
> > This patch cant be right as is.
> >
> >        if (skb_is_gso(skb)) {
> > +               if (skb_shared(skb)) {
> > +                       struct sk_buff *nskb;
> > +
> > +                       nskb = skb_clone(skb, GFP_ATOMIC);
> > +                       if (!nskb)
> > +                               return -ENOMEM;
> > +                       kfree_skb(skb); // HERE
> > +                       skb = nskb;
> > +               }
> >
> > You free original skb, while caller might dereference it later.
> > So you must pass cloned skb back to caller.
> >
> > (line 6393 :)
> >  count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first);
> >
> >
> 
> Yes, you are right. I just wonder where shared skbs are allowed. Is
> there any rule?
> 

Shared skbs are allowed for sure (pktgen is a provider of such skbs).

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote:
> Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit :
> > Yes, you are right. I just wonder where shared skbs are allowed. Is
> > there any rule?
> > 
> 
> Shared skbs are allowed for sure (pktgen is a provider of such skbs).

But pktgen doesn't use common dev_queue_xmit() path. It looks like
some places checking segmentation call pskb_expand_head() assuming skb
isn't shared. Btw, it seems ifb should have GSO features similarly to
loopback. Anyway, until all this is verified, IMHO we should revert
Changli's mirred patch in stable.

Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 10:32 +0000, Jarek Poplawski a écrit :
> On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote:
> > Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit :
> > > Yes, you are right. I just wonder where shared skbs are allowed. Is
> > > there any rule?
> > > 
> > 
> > Shared skbs are allowed for sure (pktgen is a provider of such skbs).
> 
> But pktgen doesn't use common dev_queue_xmit() path. It looks like
> some places checking segmentation call pskb_expand_head() assuming skb
> isn't shared. Btw, it seems ifb should have GSO features similarly to
> loopback. Anyway, until all this is verified, IMHO we should revert
> Changli's mirred patch in stable.
> 

I thought Changli question was : is a skb given to a device
ndo_start_xmit() handler is allowed to be shared ;)

I believe Changli patch should be reverted, because not doing a
skb_clone() should be known by caller.

To avoid the skb_clone(), we must tell caller what we did, so that
caller dont even try to reuse skb (even freeing it)

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 11:41:11AM +0100, Eric Dumazet wrote:
> Le lundi 20 décembre 2010 ?? 10:32 +0000, Jarek Poplawski a écrit :
> > On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote:
> > > Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit :
> > > > Yes, you are right. I just wonder where shared skbs are allowed. Is
> > > > there any rule?
> > > > 
> > > 
> > > Shared skbs are allowed for sure (pktgen is a provider of such skbs).
> > 
> > But pktgen doesn't use common dev_queue_xmit() path. It looks like
> > some places checking segmentation call pskb_expand_head() assuming skb
> > isn't shared. Btw, it seems ifb should have GSO features similarly to
> > loopback. Anyway, until all this is verified, IMHO we should revert
> > Changli's mirred patch in stable.
> > 
> 
> I thought Changli question was : is a skb given to a device
> ndo_start_xmit() handler is allowed to be shared ;)
> 
> I believe Changli patch should be reverted, because not doing a
> skb_clone() should be known by caller.
> 
> To avoid the skb_clone(), we must tell caller what we did, so that
> caller dont even try to reuse skb (even freeing it)

To tell the truth, Changli could assume we don't need to tell anything,
because this mirred skb is almost not shared (except the refcount ;-).

Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 11:11:41AM +0000, Jarek Poplawski wrote:
> To tell the truth, Changli could assume we don't need to tell anything,
> because this mirred skb is almost not shared (except the refcount ;-).

Btw, since there was ifb involved, I doubt this skb could hit ixgbe
xmit before unsharing, and patching the driver could matter. Anyway,
Changli, I guess, Pawel needs some instructions on this last patch?

Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Paweł Staszewski]

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

W dniu 2010-12-20 12:58, Jarek Poplawski pisze:
> On Mon, Dec 20, 2010 at 11:11:41AM +0000, Jarek Poplawski wrote:
>> To tell the truth, Changli could assume we don't need to tell anything,
>> because this mirred skb is almost not shared (except the refcount ;-).
> Btw, since there was ifb involved, I doubt this skb could hit ixgbe
> xmit before unsharing, and patching the driver could matter. Anyway,
> Changli, I guess, Pawel needs some instructions on this last patch?
>
> Jarek P.
>
>
With this patch:

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ca9036d..602cd32 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6096,6 +6096,15 @@ static int ixgbe_tso(struct ixgbe_adapter *adapter,
  	u32 mss_l4len_idx, l4len;

  	if (skb_is_gso(skb)) {
+		if (skb_shared(skb)) {
+			struct sk_buff *nskb;
+
+			nskb = skb_clone(skb, GFP_ATOMIC);
+			if (!nskb)
+				return -ENOMEM;
+			kfree_skb(skb);
+			skb = nskb;
+		}
  		if (skb_header_cloned(skb)) {
  			err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
  			if (err)


I have the same panic as without.

This patch was added to clean 2.6.37-rc6 - without previous patch for 
sch_generic.h

Attached image with kernel panic.


Thanks
Pawel



[-- Attachment #2: panic-ixgbe-patch.JPG --]
[-- Type: image/jpeg, Size: 95991 bytes --]

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 01:07:57PM +0100, Paweł Staszewski wrote:
> I have the same panic as without.
>
> This patch was added to clean 2.6.37-rc6 - without previous patch for  
> sch_generic.h
>
> Attached image with kernel panic.

This one is definitely the nicest ;-)

Thanks,
Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Changli Gao]

2010/12/20 Paweł Staszewski <pstaszewski@itcare.pl>:
> With this patch:
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index ca9036d..602cd32 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -6096,6 +6096,15 @@ static int ixgbe_tso(struct ixgbe_adapter *adapter,
>        u32 mss_l4len_idx, l4len;
>
>        if (skb_is_gso(skb)) {
> +               if (skb_shared(skb)) {
> +                       struct sk_buff *nskb;
> +
> +                       nskb = skb_clone(skb, GFP_ATOMIC);
> +                       if (!nskb)
> +                               return -ENOMEM;
> +                       kfree_skb(skb);
> +                       skb = nskb;
> +               }
>                if (skb_header_cloned(skb)) {
>                        err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
>                        if (err)
>
>
> I have the same panic as without.
>
> This patch was added to clean 2.6.37-rc6 - without previous patch for
> sch_generic.h
>
> Attached image with kernel panic.
>
>

Sigh. We have to revert my patch first. :(

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 12:22 +0000, Jarek Poplawski a écrit :
> On Mon, Dec 20, 2010 at 01:07:57PM +0100, Paweł Staszewski wrote:
> > I have the same panic as without.
> >
> > This patch was added to clean 2.6.37-rc6 - without previous patch for  
> > sch_generic.h
> >
> > Attached image with kernel panic.
> 
> This one is definitely the nicest ;-)

Indeed, we finally can see where it happens ;)

Re: Kernel panic eth2 mirred redirect to ifb0

[Eric Dumazet]

Le lundi 20 décembre 2010 à 13:45 +0100, Eric Dumazet a écrit :
> Le lundi 20 décembre 2010 à 12:22 +0000, Jarek Poplawski a écrit :
> > On Mon, Dec 20, 2010 at 01:07:57PM +0100, Paweł Staszewski wrote:
> > > I have the same panic as without.
> > >
> > > This patch was added to clean 2.6.37-rc6 - without previous patch for  
> > > sch_generic.h
> > >
> > > Attached image with kernel panic.
> > 
> > This one is definitely the nicest ;-)
> 
> Indeed, we finally can see where it happens ;)
> 

Shouldnt ifb adds some dev->features, like NETIF_F_FRAGLIST /
NETIF_F_SG ?

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 01:54:47PM +0100, Eric Dumazet wrote:
> Le lundi 20 décembre 2010 ?? 13:45 +0100, Eric Dumazet a écrit :
> > Le lundi 20 décembre 2010 ?? 12:22 +0000, Jarek Poplawski a écrit :
> > > On Mon, Dec 20, 2010 at 01:07:57PM +0100, Paweł Staszewski wrote:
> > > > I have the same panic as without.
> > > >
> > > > This patch was added to clean 2.6.37-rc6 - without previous patch for  
> > > > sch_generic.h
> > > >
> > > > Attached image with kernel panic.
> > > 
> > > This one is definitely the nicest ;-)
> > 
> > Indeed, we finally can see where it happens ;)
> > 
> 
> Shouldnt ifb adds some dev->features, like NETIF_F_FRAGLIST /
> NETIF_F_SG ?
> 

IMHO it should, (probably even more, like loopback) but we should
consider mirred can xmit to other than ifb too.

Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Jarek Poplawski]

On Mon, Dec 20, 2010 at 08:30:39PM +0800, Changli Gao wrote:
> Sigh. We have to revert my patch first. :(

Yes, please, send the patch. And don't worry: I hope you'll find the
way to add it again ;-)

Cheers,
Jarek P.

Re: Kernel panic eth2 mirred redirect to ifb0

[Changli Gao]

2010/12/20 Jarek Poplawski <jarkao2@gmail.com>:
>
> IMHO it should, (probably even more, like loopback) but we should
> consider mirred can xmit to other than ifb too.
>

I also think so. And when making ifb a multiqueue NIC, I tried to add
these dev features to ifb. :)

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

[PATCH net-next-2.6] ifb: add performance flags to dev->features

[Eric Dumazet]

Le lundi 20 décembre 2010 à 22:05 +0800, Changli Gao a écrit :
> 2010/12/20 Jarek Poplawski <jarkao2@gmail.com>:
> >
> > IMHO it should, (probably even more, like loopback) but we should
> > consider mirred can xmit to other than ifb too.
> >
> 
> I also think so. And when making ifb a multiqueue NIC, I tried to add
> these dev features to ifb. :)
> 

This has litle to do with your multiqueue work,
its more an effect of GRO being more and more deployed.

I dont see dev->features being changed in one of your previous patches.
I did dummy case in commit 6d81f41c58c6

[PATCH net-next-2.6] ifb: add performance flags to dev->features

IFB can use the full set of features flags (NETIF_F_SG |
NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
avoid unecessary split of some packets (GRO for example)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Jarek Poplawski <jarkao2@gmail.com>
Cc: Pawel Staszewski <pstaszewski@itcare.pl>
---
 drivers/net/ifb.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 124dac4..c761551 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -136,6 +136,9 @@ static void ifb_setup(struct net_device *dev)
 	ether_setup(dev);
 	dev->tx_queue_len = TX_Q_LIMIT;
 
+	dev->features |= NETIF_F_NO_CSUM | NETIF_F_SG |
+			 NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
+			 NETIF_F_TSO;
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;

Re: [PATCH net-next-2.6] ifb: add performance flags to dev->features

[Changli Gao]

On Mon, Dec 20, 2010 at 10:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>  drivers/net/ifb.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 124dac4..c761551 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -136,6 +136,9 @@ static void ifb_setup(struct net_device *dev)
>        ether_setup(dev);
>        dev->tx_queue_len = TX_Q_LIMIT;
>
> +       dev->features |= NETIF_F_NO_CSUM | NETIF_F_SG |
> +                        NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
> +                        NETIF_F_TSO;
>        dev->flags |= IFF_NOARP;
>        dev->flags &= ~IFF_MULTICAST;
>        dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
>
>
>

I think we can assign these features to vlan_features.

dev->vlan_features |= dev->features.

Although I don't know all the features related to GSO,  I just added
all of them. For pseudo NIC, I think it is safe to do so.

+       dev->features |= NETIF_F_SG | NETIF_F_NO_CSUM | NETIF_F_TSO |
+                        NETIF_F_UFO | NETIF_F_GSO_ROBUST | NETIF_F_TSO_ECN |
+                        NETIF_F_TSO6 | NETIF_F_FSO;

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH net-next-2.6] ifb: add performance flags to dev->features

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 20 Dec 2010 15:25:20 +0100

> Le lundi 20 décembre 2010 à 22:05 +0800, Changli Gao a écrit :
>> 2010/12/20 Jarek Poplawski <jarkao2@gmail.com>:
>> >
>> > IMHO it should, (probably even more, like loopback) but we should
>> > consider mirred can xmit to other than ifb too.
>> >
>> 
>> I also think so. And when making ifb a multiqueue NIC, I tried to add
>> these dev features to ifb. :)
>> 
> 
> This has litle to do with your multiqueue work,
> its more an effect of GRO being more and more deployed.
> 
> I dont see dev->features being changed in one of your previous patches.
> I did dummy case in commit 6d81f41c58c6
> 
> [PATCH net-next-2.6] ifb: add performance flags to dev->features
> 
> IFB can use the full set of features flags (NETIF_F_SG |
> NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
> avoid unecessary split of some packets (GRO for example)
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Changli has suggested that these flags can also be set in
->vlan_features, please address that if you expect me to
apply the change.

Re: [PATCH net-next-2.6] ifb: add performance flags to dev->features

[Eric Dumazet]

Le mardi 28 décembre 2010 à 13:50 -0800, David Miller a écrit :

> Changli has suggested that these flags can also be set in
> ->vlan_features, please address that if you expect me to
> apply the change.

Hmm, I am just scratching my head to actually setup vlans on top of ifb,
to test such a change ?

Re: [PATCH net-next-2.6] ifb: add performance flags to dev->features

[Jarek Poplawski]

On Tue, Dec 28, 2010 at 11:36:21PM +0100, Eric Dumazet wrote:
> Le mardi 28 décembre 2010 ?? 13:50 -0800, David Miller a écrit :
> 
> > Changli has suggested that these flags can also be set in
> > ->vlan_features, please address that if you expect me to
> > apply the change.
> 
> Hmm, I am just scratching my head to actually setup vlans on top of ifb,
> to test such a change ?

Ingress is before vlans handler so these features and the
NETIF_F_HW_VLAN_TX flag seem useful for ifb considering
dev_hard_start_xmit() checks.

Jarek P.

[PATCH v2 net-next-2.6] ifb: add performance flags

[Eric Dumazet]

Le mercredi 29 décembre 2010 à 00:07 +0100, Jarek Poplawski a écrit :

> Ingress is before vlans handler so these features and the
> NETIF_F_HW_VLAN_TX flag seem useful for ifb considering
> dev_hard_start_xmit() checks.

OK, here is v2 of the patch then, thanks everybody.


[PATCH v2 net-next-2.6] ifb: add performance flags

IFB can use the full set of features flags (NETIF_F_SG |
NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
avoid unnecessary split of some packets (GRO for example)

Changli suggested to also set vlan_features,
Jarek suggested to add NETIF_F_HW_VLAN_TX as well.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Jarek Poplawski <jarkao2@gmail.com>
Cc: Pawel Staszewski <pstaszewski@itcare.pl>
---
 drivers/net/ifb.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 124dac4..66ca7bf 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -126,6 +126,9 @@ static const struct net_device_ops ifb_netdev_ops = {
 	.ndo_validate_addr = eth_validate_addr,
 };
 
+#define IFB_FEATURES (NETIF_F_NO_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST | \
+		      NETIF_F_HIGHDMA | NETIF_F_TSO | NETIF_F_HW_VLAN_TX)
+
 static void ifb_setup(struct net_device *dev)
 {
 	/* Initialize the device structure. */
@@ -136,6 +139,9 @@ static void ifb_setup(struct net_device *dev)
 	ether_setup(dev);
 	dev->tx_queue_len = TX_Q_LIMIT;
 
+	dev->features |= IFB_FEATURES;
+	dev->vlan_features |= IFB_FEATURES;
+
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;

Re: [PATCH v2 net-next-2.6] ifb: add performance flags

[Jarek Poplawski]

On Sun, Jan 02, 2011 at 09:24:36PM +0100, Eric Dumazet wrote:
> Le mercredi 29 décembre 2010 ?? 00:07 +0100, Jarek Poplawski a écrit :
> 
> > Ingress is before vlans handler so these features and the
> > NETIF_F_HW_VLAN_TX flag seem useful for ifb considering
> > dev_hard_start_xmit() checks.
> 
> OK, here is v2 of the patch then, thanks everybody.
> 
> 
> [PATCH v2 net-next-2.6] ifb: add performance flags
> 
> IFB can use the full set of features flags (NETIF_F_SG |
> NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
> avoid unnecessary split of some packets (GRO for example)
> 
> Changli suggested to also set vlan_features,

He also suggested more GSO flags of which especially NETIF_F_TSO6
seems interesting (wrt GRO)?

Jarek P.

> Jarek suggested to add NETIF_F_HW_VLAN_TX as well.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Jarek Poplawski <jarkao2@gmail.com>
> Cc: Pawel Staszewski <pstaszewski@itcare.pl>
> ---
>  drivers/net/ifb.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 124dac4..66ca7bf 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -126,6 +126,9 @@ static const struct net_device_ops ifb_netdev_ops = {
>  	.ndo_validate_addr = eth_validate_addr,
>  };
>  
> +#define IFB_FEATURES (NETIF_F_NO_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST | \
> +		      NETIF_F_HIGHDMA | NETIF_F_TSO | NETIF_F_HW_VLAN_TX)
> +
>  static void ifb_setup(struct net_device *dev)
>  {
>  	/* Initialize the device structure. */
> @@ -136,6 +139,9 @@ static void ifb_setup(struct net_device *dev)
>  	ether_setup(dev);
>  	dev->tx_queue_len = TX_Q_LIMIT;
>  
> +	dev->features |= IFB_FEATURES;
> +	dev->vlan_features |= IFB_FEATURES;
> +
>  	dev->flags |= IFF_NOARP;
>  	dev->flags &= ~IFF_MULTICAST;
>  	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> 
> 

Re: [PATCH v2 net-next-2.6] ifb: add performance flags

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 3 Jan 2011 20:37:03 +0100

> On Sun, Jan 02, 2011 at 09:24:36PM +0100, Eric Dumazet wrote:
>> Le mercredi 29 décembre 2010 ?? 00:07 +0100, Jarek Poplawski a écrit :
>> 
>> > Ingress is before vlans handler so these features and the
>> > NETIF_F_HW_VLAN_TX flag seem useful for ifb considering
>> > dev_hard_start_xmit() checks.
>> 
>> OK, here is v2 of the patch then, thanks everybody.
>> 
>> 
>> [PATCH v2 net-next-2.6] ifb: add performance flags
>> 
>> IFB can use the full set of features flags (NETIF_F_SG |
>> NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
>> avoid unnecessary split of some packets (GRO for example)
>> 
>> Changli suggested to also set vlan_features,
> 
> He also suggested more GSO flags of which especially NETIF_F_TSO6
> seems interesting (wrt GRO)?

I think at least TSO6 would very much be appropriate here.

Re: [PATCH v2 net-next-2.6] ifb: add performance flags

[Eric Dumazet]

Le lundi 03 janvier 2011 à 11:40 -0800, David Miller a écrit :
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 3 Jan 2011 20:37:03 +0100
> 
> > On Sun, Jan 02, 2011 at 09:24:36PM +0100, Eric Dumazet wrote:
> >> Le mercredi 29 décembre 2010 ?? 00:07 +0100, Jarek Poplawski a écrit :
> >> 
> >> > Ingress is before vlans handler so these features and the
> >> > NETIF_F_HW_VLAN_TX flag seem useful for ifb considering
> >> > dev_hard_start_xmit() checks.
> >> 
> >> OK, here is v2 of the patch then, thanks everybody.
> >> 
> >> 
> >> [PATCH v2 net-next-2.6] ifb: add performance flags
> >> 
> >> IFB can use the full set of features flags (NETIF_F_SG |
> >> NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
> >> avoid unnecessary split of some packets (GRO for example)
> >> 
> >> Changli suggested to also set vlan_features,
> > 
> > He also suggested more GSO flags of which especially NETIF_F_TSO6
> > seems interesting (wrt GRO)?
> 
> I think at least TSO6 would very much be appropriate here.

Yes, why not, I am only wondering why loopback / dummy (and others ?)
only set NETIF_F_TSO :)

Since I want to play with ECN, I might also add NETIF_F_TSO_ECN ;)

For other flags, I really doubt it can matter on ifb ?

[PATCH v3 net-next-2.6] ifb: add performance flags

IFB can use the full set of features flags (NETIF_F_SG |
NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
avoid unnecessary split of some packets (GRO for example)

Changli suggested to also set vlan_features, NETIF_F_TSO6,
NETIF_F_TSO_ECN.

Jarek suggested to add NETIF_F_HW_VLAN_TX as well.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Jarek Poplawski <jarkao2@gmail.com>
Cc: Pawel Staszewski <pstaszewski@itcare.pl>
---
 drivers/net/ifb.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
index 124dac4..e07d487 100644
--- a/drivers/net/ifb.c
+++ b/drivers/net/ifb.c
@@ -126,6 +126,10 @@ static const struct net_device_ops ifb_netdev_ops = {
 	.ndo_validate_addr = eth_validate_addr,
 };
 
+#define IFB_FEATURES (NETIF_F_NO_CSUM | NETIF_F_SG  | NETIF_F_FRAGLIST	| \
+		      NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6	| \
+		      NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_TX)
+
 static void ifb_setup(struct net_device *dev)
 {
 	/* Initialize the device structure. */
@@ -136,6 +140,9 @@ static void ifb_setup(struct net_device *dev)
 	ether_setup(dev);
 	dev->tx_queue_len = TX_Q_LIMIT;
 
+	dev->features |= IFB_FEATURES;
+	dev->vlan_features |= IFB_FEATURES;
+
 	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;

Re: [PATCH v2 net-next-2.6] ifb: add performance flags

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 03 Jan 2011 21:35:22 +0100

> Le lundi 03 janvier 2011 à 11:40 -0800, David Miller a écrit :
>> I think at least TSO6 would very much be appropriate here.
> 
> Yes, why not, I am only wondering why loopback / dummy (and others ?)
> only set NETIF_F_TSO :)

TSO6 probably didn't exist when the current set were added,
at least in the loopback case that's almost certainly the
reason.

> Since I want to play with ECN, I might also add NETIF_F_TSO_ECN ;)
> 
> For other flags, I really doubt it can matter on ifb ?
> 
> [PATCH v3 net-next-2.6] ifb: add performance flags
> 
> IFB can use the full set of features flags (NETIF_F_SG |
> NETIF_F_FRAGLIST | NETIF_F_TSO | NETIF_F_NO_CSUM | NETIF_F_HIGHDMA) to
> avoid unnecessary split of some packets (GRO for example)
> 
> Changli suggested to also set vlan_features, NETIF_F_TSO6,
> NETIF_F_TSO_ECN.
> 
> Jarek suggested to add NETIF_F_HW_VLAN_TX as well.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

I'll apply this, thanks Eric.