netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] bridge: inherit slave devices needed_headroom
@ 2013-08-20  9:21 Florian Fainelli
  2013-08-22  3:41 ` David Miller
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Florian Fainelli @ 2013-08-20  9:21 UTC (permalink / raw)
  To: netdev; +Cc: stephen, eric.dumazet, vyasevic, davem, Florian Fainelli

Some slave devices may have set a dev->needed_headroom value which is
different than the default one, most likely in order to prepend a
hardware descriptor in front of the Ethernet frame to send. Whenever a
new slave is added to a bridge, ensure that we update the
needed_headroom value accordingly to account for the slave
needed_headroom value.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/bridge/br_if.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index aa6c9a8..c41d5fb 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -383,6 +383,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	netdev_update_features(br->dev);
 
+	if (br->dev->needed_headroom < dev->needed_headroom)
+		br->dev->needed_headroom = dev->needed_headroom;
+
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);
 
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] bridge: inherit slave devices needed_headroom
  2013-08-20  9:21 [RFC PATCH] bridge: inherit slave devices needed_headroom Florian Fainelli
@ 2013-08-22  3:41 ` David Miller
  2013-08-22  4:58 ` Stephen Hemminger
  2013-08-23  2:49 ` Amos Kong
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-08-22  3:41 UTC (permalink / raw)
  To: f.fainelli; +Cc: netdev, stephen, eric.dumazet, vyasevic

From: "Florian Fainelli" <f.fainelli@gmail.com>
Date: Tue, 20 Aug 2013 10:21:45 +0100

> Some slave devices may have set a dev->needed_headroom value which is
> different than the default one, most likely in order to prepend a
> hardware descriptor in front of the Ethernet frame to send. Whenever a
> new slave is added to a bridge, ensure that we update the
> needed_headroom value accordingly to account for the slave
> needed_headroom value.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

This looks fine to me, but I'd like to see either Eric, Stephen, or
Vlad review this before I apply it.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] bridge: inherit slave devices needed_headroom
  2013-08-20  9:21 [RFC PATCH] bridge: inherit slave devices needed_headroom Florian Fainelli
  2013-08-22  3:41 ` David Miller
@ 2013-08-22  4:58 ` Stephen Hemminger
  2013-08-22 20:06   ` David Miller
  2013-08-23  2:49 ` Amos Kong
  2 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2013-08-22  4:58 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev, eric.dumazet, vyasevic, davem

On Tue, 20 Aug 2013 10:21:45 +0100
"Florian Fainelli" <f.fainelli@gmail.com> wrote:

> Some slave devices may have set a dev->needed_headroom value which is
> different than the default one, most likely in order to prepend a
> hardware descriptor in front of the Ethernet frame to send. Whenever a
> new slave is added to a bridge, ensure that we update the
> needed_headroom value accordingly to account for the slave
> needed_headroom value.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/bridge/br_if.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index aa6c9a8..c41d5fb 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -383,6 +383,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>  
>  	netdev_update_features(br->dev);
>  
> +	if (br->dev->needed_headroom < dev->needed_headroom)
> +		br->dev->needed_headroom = dev->needed_headroom;
> +
>  	spin_lock_bh(&br->lock);
>  	changed_addr = br_stp_recalculate_bridge_id(br);
>  

I am okay with this but it only helps locally generated traffic
which is the minority on most bridge devices. It does nothing for the case
where one port needs more headroom than the packet received on the
other port. That is why a device has to always work when it receives
a packet with less headroom, usually by copying.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] bridge: inherit slave devices needed_headroom
  2013-08-22  4:58 ` Stephen Hemminger
@ 2013-08-22 20:06   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-08-22 20:06 UTC (permalink / raw)
  To: stephen; +Cc: f.fainelli, netdev, eric.dumazet, vyasevic

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Wed, 21 Aug 2013 21:58:29 -0700

> On Tue, 20 Aug 2013 10:21:45 +0100
> "Florian Fainelli" <f.fainelli@gmail.com> wrote:
> 
>> Some slave devices may have set a dev->needed_headroom value which is
>> different than the default one, most likely in order to prepend a
>> hardware descriptor in front of the Ethernet frame to send. Whenever a
>> new slave is added to a bridge, ensure that we update the
>> needed_headroom value accordingly to account for the slave
>> needed_headroom value.
>> 
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/bridge/br_if.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index aa6c9a8..c41d5fb 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -383,6 +383,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>  
>>  	netdev_update_features(br->dev);
>>  
>> +	if (br->dev->needed_headroom < dev->needed_headroom)
>> +		br->dev->needed_headroom = dev->needed_headroom;
>> +
>>  	spin_lock_bh(&br->lock);
>>  	changed_addr = br_stp_recalculate_bridge_id(br);
>>  
> 
> I am okay with this but it only helps locally generated traffic
> which is the minority on most bridge devices. It does nothing for the case
> where one port needs more headroom than the packet received on the
> other port. That is why a device has to always work when it receives
> a packet with less headroom, usually by copying.
> 

Right so this is basically an optimization.

Florian please submit this formally as a non-RFC patch so Stephen
can ACK it and I can apply it, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] bridge: inherit slave devices needed_headroom
  2013-08-20  9:21 [RFC PATCH] bridge: inherit slave devices needed_headroom Florian Fainelli
  2013-08-22  3:41 ` David Miller
  2013-08-22  4:58 ` Stephen Hemminger
@ 2013-08-23  2:49 ` Amos Kong
  2013-08-27 11:04   ` Florian Fainelli
  2 siblings, 1 reply; 6+ messages in thread
From: Amos Kong @ 2013-08-23  2:49 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, stephen, eric.dumazet, Vlad Yasevich, David Miller

On Tue, Aug 20, 2013 at 5:21 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Some slave devices may have set a dev->needed_headroom value which is
> different than the default one, most likely in order to prepend a
> hardware descriptor in front of the Ethernet frame to send. Whenever a
> new slave is added to a bridge, ensure that we update the
> needed_headroom value accordingly to account for the slave
> needed_headroom value.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/bridge/br_if.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index aa6c9a8..c41d5fb 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -383,6 +383,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>
>         netdev_update_features(br->dev);
>
> +       if (br->dev->needed_headroom < dev->needed_headroom)
> +               br->dev->needed_headroom = dev->needed_headroom;
> +

do we need to update 'br->dev->needed_headroom' in br_del_if()?

if (br->dev->needed_headroom == dev->needed_headroom)
    br->dev->needed_headroom = ....;


>         spin_lock_bh(&br->lock);
>         changed_addr = br_stp_recalculate_bridge_id(br);
>
> --
> 1.8.1.2
>
>
> --
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] bridge: inherit slave devices needed_headroom
  2013-08-23  2:49 ` Amos Kong
@ 2013-08-27 11:04   ` Florian Fainelli
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2013-08-27 11:04 UTC (permalink / raw)
  To: Amos Kong
  Cc: netdev, Stephen Hemminger, Eric Dumazet, Vlad Yasevich,
	David Miller

2013/8/23 Amos Kong <kongjianjun@gmail.com>:
> On Tue, Aug 20, 2013 at 5:21 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> Some slave devices may have set a dev->needed_headroom value which is
>> different than the default one, most likely in order to prepend a
>> hardware descriptor in front of the Ethernet frame to send. Whenever a
>> new slave is added to a bridge, ensure that we update the
>> needed_headroom value accordingly to account for the slave
>> needed_headroom value.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/bridge/br_if.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index aa6c9a8..c41d5fb 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -383,6 +383,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
>>
>>         netdev_update_features(br->dev);
>>
>> +       if (br->dev->needed_headroom < dev->needed_headroom)
>> +               br->dev->needed_headroom = dev->needed_headroom;
>> +
>
> do we need to update 'br->dev->needed_headroom' in br_del_if()?
>
> if (br->dev->needed_headroom == dev->needed_headroom)
>     br->dev->needed_headroom = ....;

In theory yes, we should recompute the minimum headroom which is
common to all bridge members. This is something that I want to address
separately though with the NETDEV_CHANGEROOM notifier.
-- 
Florian

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-08-27 11:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-20  9:21 [RFC PATCH] bridge: inherit slave devices needed_headroom Florian Fainelli
2013-08-22  3:41 ` David Miller
2013-08-22  4:58 ` Stephen Hemminger
2013-08-22 20:06   ` David Miller
2013-08-23  2:49 ` Amos Kong
2013-08-27 11:04   ` Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).