From: "Yuval Mintz" <yuvalmin@broadcom•com>
To: "Eric Dumazet" <eric.dumazet@gmail•com>
Cc: "David Miller" <davem@davemloft•net>,
netdev@vger•kernel.org, "Eilon Greenstein" <eilong@broadcom•com>,
ariele@broadcom•com
Subject: Re: [PATCH net-next] bnx2x: fix GRO parameters
Date: Wed, 16 Jan 2013 16:38:40 +0200 [thread overview]
Message-ID: <50F6BB70.8070409@broadcom.com> (raw)
In-Reply-To: <1358350146.19956.658.camel@edumazet-glaptop>
>>> -static u16 bnx2x_set_lro_mss(struct bnx2x *bp, u16 parsing_flags,
>>> - u16 len_on_bd)
>>> +static void bnx2x_set_gro_params(struct sk_buff *skb, struct bnx2x *bp,
>>> + u16 parsing_flags, u16 len_on_bd,
>>> + unsigned int pkt_len)
>>
>> This is purely semantic, but our convention is for `struct bnx2x' to be
>> the first argument in our functions.
>>
>>> {
>>> /*
>>> - * TPA arrgregation won't have either IP options or TCP options
>>> + * TPA aggregation won't have either IP options or TCP options
>>> * other than timestamp or IPv6 extension headers.
>>> */
>>> u16 hdrs_len = ETH_HLEN + sizeof(struct tcphdr);
>>
>> TPA_MODE_LRO indicates that an LRO aggregation was made by our FW. It seems
>> like your patch eliminates the difference in configuration between the two
>> (GRO and LRO)
>>
>
> Thats the case, since you call GRO functions even if 'LRO ' is ON
>
> I specifically had to remove the tests you guys do.
>
>> perhaps instead we should do something like:
>>
>> + static void bnx2x_set_lro_params(struct bnx2x *bp, struct sk_buff *skb,
>> + u16 parsing_flags, u16 len_on_bd,
>> + unsigned int pkt_len,
>> + bnx2x_tpa_mode_t mode)
>>
>> And arrange its suggested code so that only gso_size will be set for LRO.
>>
>
> I fail to understand why adding a conditional would change something.
>
> Setting it is needed for GRO as well.
>
I was a bit unclear - I meant the for LRO, only gso_size will be set
(while for GRO it will be set as well as the additional GRO fields)
>>>
>>> if (GET_FLAG(parsing_flags, PARSING_FLAGS_OVER_ETHERNET_PROTOCOL) ==
>>> - PRS_FLAG_OVERETH_IPV6)
>>> + PRS_FLAG_OVERETH_IPV6) {
>>> hdrs_len += sizeof(struct ipv6hdr);
>>> - else /* IPv4 */
>>> + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
>>> + } else {
>>> hdrs_len += sizeof(struct iphdr);
>>> -
>>> + skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
>>> + }
>>>
>>
>>
>>
>>> #ifdef BNX2X_STOP_ON_ERROR
>>> @@ -651,7 +655,7 @@ static void bnx2x_gro_receive(struct bnx2x *bp, struct bnx2x_fastpath *fp,
>>> struct sk_buff *skb)
>>> {
>>> #ifdef CONFIG_INET
>>> - if (fp->mode == TPA_MODE_GRO && skb_shinfo(skb)->gso_size) {
>>> + if (skb_shinfo(skb)->gso_size) {
>>
>> This also seems like an incorrect removal, as TPA_MODE_LRO is (again)
>> a feasible option, and we wouldn't want the a `gro_complete' here.
>>
>
>
> Problem is : You call GRO functions, faking a GRO packet.
>
> You must therefore exactly present same attributes in skb than after a
> true software GRO step.
>
> I did my tests booting a standard driver, that is with LRO on.
I think we have a misunderstanding here - when LRO is on, our FW works in
TPA_MODE_LRO (LRO/GRO FW are mutually exclusive). In that mode, the bnx2x
driver will not try to 'fake' GRO packets in the same manner.
next prev parent reply other threads:[~2013-01-16 15:37 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-14 15:11 [PATCH net-next 0/10] bnx2x: patch series Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 01/10] bnx2x: Clear dirty status when booting after UNDI Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 02/10] bnx2x: Add an additional fatal hw assertion - BRB_HW_INTERRUPT Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 03/10] bnx2x: use SAN Mac for FCoE Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 04/10] bnx2x: Fix rare self-test failures Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 05/10] bnx2x: Added nvram personalities support Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 06/10] bnx2x: add `ethtool -w' support Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 07/10] bnx2x: improve stop-on-error Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 08/10] bnx2x: Clean previous IGU status before ack Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support Yuval Mintz
2013-01-14 17:22 ` Eric Dumazet
2013-01-14 18:44 ` Eric Dumazet
2013-01-15 7:28 ` Yuval Mintz
2013-01-15 14:39 ` Eric Dumazet
2013-01-15 14:02 ` Yuval Mintz
2013-01-15 15:07 ` Eric Dumazet
2013-01-15 20:09 ` David Miller
2013-01-16 4:56 ` Eric Dumazet
2013-01-16 5:37 ` [PATCH net-next] bnx2x: fix GRO parameters Eric Dumazet
2013-01-16 7:01 ` Yuval Mintz
2013-01-16 15:29 ` Eric Dumazet
2013-01-16 14:38 ` Yuval Mintz [this message]
2013-01-16 16:50 ` Eric Dumazet
2013-01-17 7:16 ` Yuval Mintz
2013-01-14 15:11 ` [PATCH net-next 10/10] bnx2x: Introduce 2013 and advance version to 1.78.02 Yuval Mintz
-- strict thread matches above, loose matches on Subject: below --
2013-01-17 13:26 [PATCH net-next] bnx2x: fix GRO parameters Yuval Mintz
2013-01-17 14:33 ` Eric Dumazet
2013-01-17 19:56 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50F6BB70.8070409@broadcom.com \
--to=yuvalmin@broadcom$(echo .)com \
--cc=ariele@broadcom$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=eilong@broadcom$(echo .)com \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox