public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed
From: roopa <roopa@cumulusnetworks•com>
To: Scott Feldman <sfeldma@gmail•com>
Cc: "David S. Miller" <davem@davemloft•net>,
	"Jiří Pírko" <jiri@resnulli•us>,
	"Arad, Ronen" <ronen.arad@intel•com>,
	Netdev <netdev@vger•kernel.org>
Subject: Re: [PATCH net-next RFC v2] switchdev: bridge: drop hardware forwarded packets
Date: Fri, 20 Mar 2015 14:20:54 -0700	[thread overview]
Message-ID: <550C8F36.9030800@cumulusnetworks.com> (raw)
In-Reply-To: <CAE4R7bCXTB0eZdyT9+a2FbJvy4K7QHpRzR4WvRGMryE-QZN8zQ@mail.gmail.com>

On 3/20/15, 11:03 AM, Scott Feldman wrote:
> On Fri, Mar 20, 2015 at 9:58 AM,  <roopa@cumulusnetworks•com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks•com>
>>
>> On a Linux bridge with bridge forwarding offloaded to switch ASIC,
>> there is a need to not re-forward frames that have already been
>> forwarded in hardware.
>>
>> Typically these are broadcast or multicast frames forwarded by the
>> hardware to multiple destination ports including sending a copy of
>> the packet to the cpu (kernel e.g. an arp broadcast).
>> The bridge driver will try to forward the packet again, resulting in
>> two copies of the same packet.
>>
>> These packets can also come up to the kernel for logging when they hit
>> a LOG acl rule in hardware. In such cases, you do want the packet
>> to go through the bridge netfilter hooks. Hence, this patch adds the
>> required checks just before the packet is being xmited.
>>
>> v2:
>>          - Add a new hw_fwded flag in skbuff to indicate that the packet
>>          is already hardware forwarded. Switch driver will set this flag.
>>          I have been trying to avoid having this flag in the skb
>>          and thats why this patch has been in my tree for long. Cant think
>>          of other better alternatives. Suggestions are welcome. I have put
>>          this under CONFIG_NET_SWITCHDEV to minimize the impact.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks•com>
>> Signed-off-by: Wilson Kok <wkok@cumulusnetworks•com>
>> ---
>>   include/linux/skbuff.h  |    7 +++++--
>>   net/bridge/br_forward.c |   11 +++++++++++
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index bba1330..1973b5c 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -560,8 +560,11 @@ struct sk_buff {
>>                                  fclone:2,
>>                                  peeked:1,
>>                                  head_frag:1,
>> -                               xmit_more:1;
>> -       /* one bit hole */
>> +                               xmit_more:1,
>> +#ifdef CONFIG_NET_SWITCHDEV
>> +                               hw_fwded:1;
>> +#endif
>> +       /* one bit hole if CONFIG_NET_SWITCHDEV not defined */
> Did you want this flag not copied in __copy_skb_header()?  Seems those
> flags are special cased.  There is room for a bit here:
>
> #ifdef CONFIG_IPV6_NDISC_NODETYPE
>          __u8                    ndisc_nodetype:2;
> #endif
>          __u8                    ipvs_property:1;
>          __u8                    inner_protocol_type:1;
>          __u8                    remcsum_offload:1;
>          /* 3 or 5 bit hole */
>           <<<<<<<<

thx, yes I can add it here.
(I found the first 1 bit hole and added it there).
>
>>          kmemcheck_bitfield_end(flags1);
>>
>>          /* fields enclosed in headers_start/headers_end are copied
>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>> index 3304a54..b60b96e 100644
>> --- a/net/bridge/br_forward.c
>> +++ b/net/bridge/br_forward.c
>> @@ -37,6 +37,17 @@ static inline int should_deliver(const struct net_bridge_port *p,
>>
>>   int br_dev_queue_push_xmit(struct sk_buff *skb)
>>   {
>> +
>> +#ifdef CONFIG_NET_SWITCHDEV
>> +       /* If skb is already hw forwarded and the port being forwarded
>> +        * to is a switch port, dont reforward
>> +        */
>> +       if (skb->hw_fwded && (skb->dev->features & NETIF_F_HW_SWITCH_OFFLOAD)) {
> The  check for skb->dev->features & NETIF_F_HW_SWITCH_OFFLOAD is redundant.

The skb->dev is the device it is getting forwarded to. The hw_fwded flag 
was set by the
device that originated the skb. The NETIF_F_HW_SWITCH_OFFLOAD flag is 
required because
the device being forwarded to can be a non switch port.

thanks,
Roopa

  reply	other threads:[~2015-03-20 21:20 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-20 16:58 [PATCH net-next RFC v2] switchdev: bridge: drop hardware forwarded packets roopa
2015-03-20 17:11 ` John Fastabend
2015-03-20 18:13   ` Scott Feldman
2015-03-20 18:30     ` John Fastabend
2015-03-20 22:06     ` roopa
2015-03-20 22:37       ` Scott Feldman
2015-03-20 23:30         ` roopa
2015-03-21  0:26           ` Scott Feldman
2015-03-21  5:53             ` roopa
2015-03-20 21:03   ` roopa
2015-03-20 21:23     ` John Fastabend
2015-03-20 22:04       ` Andrew Lunn
2015-03-20 23:12       ` roopa
2015-03-20 18:03 ` Scott Feldman
2015-03-20 21:20   ` roopa [this message]
2015-03-20 20:36 ` David Miller
2015-03-20 21:36   ` roopa
2015-03-20 22:09     ` Andrew Lunn
2015-03-20 23:43       ` Florian Fainelli
2015-03-23  0:22       ` Guenter Roeck
2015-03-23  1:33         ` John Fastabend
2015-03-23  2:57           ` Guenter Roeck
2015-03-23  3:18             ` John Fastabend
2015-03-23  3:33               ` Guenter Roeck
2015-03-23 17:12                 ` roopa
2015-03-24  5:59                   ` Scott Feldman
2015-03-24 13:13                     ` Guenter Roeck
2015-03-24 18:08                       ` Scott Feldman
2015-03-24 14:29                     ` Jiri Pirko
2015-03-24 16:01                       ` Guenter Roeck
2015-03-24 17:45                         ` roopa
2015-03-24 17:58                           ` Guenter Roeck
2015-03-24 18:14                             ` Scott Feldman
2015-03-25  3:10                               ` Guenter Roeck
2015-03-25  3:46                               ` Florian Fainelli
2015-03-25  5:06                                 ` Scott Feldman
2015-03-25 17:01                                   ` roopa
2015-03-26  7:44                                     ` Scott Feldman
2015-03-26  8:20                                       ` Jiri Pirko
2015-03-26 14:28                                         ` Scott Feldman
2015-03-26 14:49                                           ` Jiri Pirko
2015-03-27  1:08                                             ` Simon Horman
2015-03-27  6:02                                               ` Jiri Pirko
2015-03-27  6:43                                             ` Scott Feldman
2015-03-27  7:01                                               ` Jiri Pirko
2015-03-27 23:19                                                 ` Scott Feldman
2015-03-30 14:06                                       ` roopa
2015-03-24 18:48                             ` David Christensen
2015-03-24 17:58                         ` Scott Feldman
2015-03-23 17:10           ` roopa
2015-03-23 14:00         ` roopa

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=550C8F36.9030800@cumulusnetworks.com \
    --to=roopa@cumulusnetworks$(echo .)com \
    --cc=davem@davemloft$(echo .)net \
    --cc=jiri@resnulli$(echo .)us \
    --cc=netdev@vger$(echo .)kernel.org \
    --cc=ronen.arad@intel$(echo .)com \
    --cc=sfeldma@gmail$(echo .)com \
    /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