bridge: Enable configuration of ageing interval for bridges and switch devices.

On Fri, 14 Aug 2015, Premkumar Jonnala wrote:

> Bridge devices have ageing interval used to age out MAC addresses
> from FDB.  This ageing interval was not configuratble.
>
> Enable netlink based configuration of ageing interval for bridges and
> switch devices.  The ageing interval changes the timer used to purge
> inactive FDB entries in bridges.  The ageing interval config is
> propagated to switch devices, so that platform or hardware based
> ageing works according to configuration.
>
> Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

Hi Premkumar,

I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

I hope you don't mind if share a patch for setting bridge-level attributes 
down to the switch hardware ports.  It uses the switchdev_port_attr_set() 
call on the bridge interface itself when any IFLA_BR_xxx attribute is set 
via netlink.   switchdev_port_attr_set() will do a recursive walk of all 
lower devs from the bridge, stopping at each to set the attribute.  I 
added one small tweak to switchdev_port_attr_set() to allow skipping over 
ports that return -EOPNOTSUPP.  The advantage to using 
switchdev_port_attr_set() is it automatically knows how to find the leaf 
ports in a stacked driver setup, and it uses the prepare/commit 
transaction model to make sure all ports can support new attr setting 
before committing setting to hardware.

I gave an example using rocker to set IFLA_BR_AGEING_TIME.  It's stubbed 
out, but you get the idea.  Other IFLA_BR_xxx attrs can be added to this 
model.

Does this look like something that would work?  If so, would you mind 
running with this patch and maybe even extending it for other IFLA_BR_xxx 
attrs?

Again, I hope I'm not stepping on toes here by rewriting your patch, but 
it was the best way to communicate the idea of how to use the switchdev 
frame work for these bridge attrs.

-scott




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

Hello Scott,

Thank you for the diff and comments.   Please see my comments inline.

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Tuesday, August 18, 2015 12:48 PM
> To: Premkumar Jonnala
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> 
> 
> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> 
> > Bridge devices have ageing interval used to age out MAC addresses
> > from FDB.  This ageing interval was not configuratble.
> >
> > Enable netlink based configuration of ageing interval for bridges and
> > switch devices.  The ageing interval changes the timer used to purge
> > inactive FDB entries in bridges.  The ageing interval config is
> > propagated to switch devices, so that platform or hardware based
> > ageing works according to configuration.
> >
> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
> 
> Hi Premkumar,
> 
> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

What is the motivation for using 'ip link' command to configure bridge attributes?  IMHO,
bridge command is better suited for that. 

> 
> I hope you don't mind if share a patch for setting bridge-level attributes
> down to the switch hardware ports.  It uses the switchdev_port_attr_set()
> call on the bridge interface itself when any IFLA_BR_xxx attribute is set
> via netlink.   switchdev_port_attr_set() will do a recursive walk of all
> lower devs from the bridge, stopping at each to set the attribute.  I
> added one small tweak to switchdev_port_attr_set() to allow skipping over
> ports that return -EOPNOTSUPP.  The advantage to using
> switchdev_port_attr_set() is it automatically knows how to find the leaf
> ports in a stacked driver setup, and it uses the prepare/commit
> transaction model to make sure all ports can support new attr setting
> before committing setting to hardware.
> 
> I gave an example using rocker to set IFLA_BR_AGEING_TIME.  It's stubbed
> out, but you get the idea.  Other IFLA_BR_xxx attrs can be added to this
> model.
> 
> Does this look like something that would work?  If so, would you mind
> running with this patch and maybe even extending it for other IFLA_BR_xxx
> attrs?

Sure, let me go with this patch.  Couple of comments inline.

> 
> Again, I hope I'm not stepping on toes here by rewriting your patch, but
> it was the best way to communicate the idea of how to use the switchdev
> frame work for these bridge attrs.
> 
> -scott
> 
> 
> 
> 
> diff --git a/drivers/net/ethernet/rocker/rocker.c
> b/drivers/net/ethernet/rocker/rocker.c
> index 0cc9e8f..830f8e6 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -4680,6 +4680,24 @@ static int rocker_port_brport_flags_set(struct
> rocker_port *rocker_port,
>   	return err;
>   }
> 
> +static int rocker_port_bridge_set(struct rocker_port *rocker_port,
> +				  enum switchdev_trans trans,
> +				  struct switchdev_attr_bridge *bridge)
> +{
> +	switch (bridge->attr) {
> +	case IFLA_BR_AGEING_TIME:
> +		{
> +			u32 ageing_time = bridge->val;
> +			/* XXX push ageing_time down to device */
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>   static int rocker_port_attr_set(struct net_device *dev,
>   				struct switchdev_attr *attr)
>   {
> @@ -4707,6 +4725,10 @@ static int rocker_port_attr_set(struct net_device
> *dev,
>   		err = rocker_port_brport_flags_set(rocker_port, attr->trans,
>   						   attr->u.brport_flags);
>   		break;
> +	case SWITCHDEV_ATTR_BRIDGE:
> +		err = rocker_port_bridge_set(rocker_port, attr->trans,
> +					     &attr->u.bridge);
> +		break;
>   	default:
>   		err = -EOPNOTSUPP;
>   		break;
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 319baab..22a6dbe 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -15,6 +15,7 @@
>   #include <linux/notifier.h>
> 
>   #define SWITCHDEV_F_NO_RECURSE		BIT(0)
> +#define SWITCHDEV_F_SKIP_EOPNOTSUPP	BIT(1)
> 
>   enum switchdev_trans {
>   	SWITCHDEV_TRANS_NONE,
> @@ -28,6 +29,7 @@ enum switchdev_attr_id {
>   	SWITCHDEV_ATTR_PORT_PARENT_ID,
>   	SWITCHDEV_ATTR_PORT_STP_STATE,
>   	SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS,
> +	SWITCHDEV_ATTR_BRIDGE,
>   };
> 
>   struct switchdev_attr {
> @@ -38,6 +40,10 @@ struct switchdev_attr {
>   		struct netdev_phys_item_id ppid;	/* PORT_PARENT_ID
> */
>   		u8 stp_state;				/* PORT_STP_STATE
> */
>   		unsigned long brport_flags;		/*
> PORT_BRIDGE_FLAGS */
> +		struct switchdev_attr_bridge {		/* BRIDGE */
> +			enum ifla_br attr;
> +			u32 val;
> +		} bridge;
>   	} u;
>   };
> 
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index ff659f0..0630053 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -222,7 +222,7 @@ enum in6_addr_gen_mode {
> 
>   /* Bridge section */
> 
> -enum {
> +enum ifla_br {
>   	IFLA_BR_UNSPEC,
>   	IFLA_BR_FORWARD_DELAY,
>   	IFLA_BR_HELLO_TIME,
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 0f2408f..01401ea 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev, struct
> nlattr *tb[],
>   	}
> 
>   	if (data[IFLA_BR_AGEING_TIME]) {
> -		u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);

Should we do some range checking here to ensure that the value is within a certain range.
IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million seconds.

> -
> -		br->ageing_time = clock_t_to_jiffies(ageing_time);
> +		err = br_set_ageing_time(br,
> nla_get_u32(data[IFLA_BR_AGEING_TIME]));
> +		if (err)
> +			return err;
>   	}
> 
>   	if (data[IFLA_BR_STP_STATE]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 3d95647..9807a6f 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -807,6 +807,7 @@ void __br_set_forward_delay(struct net_bridge *br,
> unsigned long t);
>   int br_set_forward_delay(struct net_bridge *br, unsigned long x);
>   int br_set_hello_time(struct net_bridge *br, unsigned long x);
>   int br_set_max_age(struct net_bridge *br, unsigned long x);
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
> 
> 
>   /* br_stp_if.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index ed74ffa..5de27bc 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,25 @@ int br_set_max_age(struct net_bridge *br, unsigned
> long val)
> 
>   }
> 
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> +{
> +	struct switchdev_attr attr = {
> +		.id = SWITCHDEV_ATTR_BRIDGE,
> +		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> +		.u.bridge.attr = IFLA_BR_AGEING_TIME,
> +		.u.bridge.val = ageing_time,
> +	};
> +	int err;
> +
> +	err = switchdev_port_attr_set(br->dev, &attr);
> +	if (err)
> +		return err;
> +
> +	br->ageing_time = clock_t_to_jiffies(ageing_time);

Should we restart the timer here the new time takes effect?

> +
> +	return 0;
> +}
> +
>   void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
>   {
>   	br->bridge_forward_delay = t;
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 16c1c43..b990301 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -73,7 +73,7 @@ static int __switchdev_port_attr_set(struct net_device
> *dev,
>   		return ops->switchdev_port_attr_set(dev, attr);
> 
>   	if (attr->flags & SWITCHDEV_F_NO_RECURSE)
> -		return err;
> +		goto done;
> 
>   	/* Switch device port(s) may be stacked under
>   	 * bond/team/vlan dev, so recurse down to set attr on
> @@ -82,10 +82,17 @@ static int __switchdev_port_attr_set(struct net_device
> *dev,
> 
>   	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>   		err = __switchdev_port_attr_set(lower_dev, attr);
> +		if (err == -EOPNOTSUPP &&
> +		    attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
> +			continue;
>   		if (err)
>   			break;
>   	}
> 
> +done:
> +	if (err == -EOPNOTSUPP && attr->flags &
> SWITCHDEV_F_SKIP_EOPNOTSUPP)
> +		err = 0;
> +
>   	return err;
>   }

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

On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
> Hello Scott,
>
> Thank you for the diff and comments.   Please see my comments inline.
>
>> -----Original Message-----
>> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> Sent: Tuesday, August 18, 2015 12:48 PM
>> To: Premkumar Jonnala
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
>> and switch devices.
>>
>>
>>
>> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>>
>> > Bridge devices have ageing interval used to age out MAC addresses
>> > from FDB.  This ageing interval was not configuratble.
>> >
>> > Enable netlink based configuration of ageing interval for bridges and
>> > switch devices.  The ageing interval changes the timer used to purge
>> > inactive FDB entries in bridges.  The ageing interval config is
>> > propagated to switch devices, so that platform or hardware based
>> > ageing works according to configuration.
>> >
>> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>>
>> Hi Premkumar,
>>
>> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>
> What is the motivation for using 'ip link' command to configure bridge attributes?  IMHO,
> bridge command is better suited for that.

Can you extend bridge command to allow setting/getting these bridge
attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
changes needed to the kernel.

bridge link set dev br0 ageing_time 1000

 --or--

ip link set dev br0 type bridge ageing_time 1000

>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 0f2408f..01401ea 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev, struct
>> nlattr *tb[],
>>       }
>>
>>       if (data[IFLA_BR_AGEING_TIME]) {
>> -             u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
>
> Should we do some range checking here to ensure that the value is within a certain range.
> IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million seconds.

Sure, but make that a separate patch.

>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> +{
>> +     struct switchdev_attr attr = {
>> +             .id = SWITCHDEV_ATTR_BRIDGE,
>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> +             .u.bridge.attr = IFLA_BR_AGEING_TIME,
>> +             .u.bridge.val = ageing_time,
>> +     };
>> +     int err;
>> +
>> +     err = switchdev_port_attr_set(br->dev, &attr);
>> +     if (err)
>> +             return err;
>> +
>> +     br->ageing_time = clock_t_to_jiffies(ageing_time);
>
> Should we restart the timer here the new time takes effect?

I don't know...I just copied what the original code did.  If it does
need to be restarted, break that out as a separate patch.

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

> -----Original Message-----

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]

> On Behalf Of Scott Feldman

> Sent: Wednesday, August 19, 2015 12:54 PM

> To: Premkumar Jonnala

> Cc: netdev@vger.kernel.org

> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges

> and switch devices.

> 

> On Wed, Aug 19, 2015 at 2:34 AM, Premkumar Jonnala

> <pjonnala@broadcom.com> wrote:

> > Hello Scott,

> >

> > Thank you for the diff and comments.   Please see my comments inline.

> >

> >> -----Original Message-----

> >> From: Scott Feldman [mailto:sfeldma@gmail.com]

> >> Sent: Tuesday, August 18, 2015 12:48 PM

> >> To: Premkumar Jonnala

> >> Cc: netdev@vger.kernel.org

> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval

> >> for bridges and switch devices.

> >>

> >>

> >>

> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:

> >>

> >> > Bridge devices have ageing interval used to age out MAC addresses

> >> > from FDB.  This ageing interval was not configuratble.

> >> >

> >> > Enable netlink based configuration of ageing interval for bridges

> >> > and switch devices.  The ageing interval changes the timer used to

> >> > purge inactive FDB entries in bridges.  The ageing interval config

> >> > is propagated to switch devices, so that platform or hardware based

> >> > ageing works according to configuration.

> >> >

> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

> >>

> >> Hi Premkumar,

> >>

> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

> >

> > What is the motivation for using 'ip link' command to configure bridge

> > attributes?  IMHO, bridge command is better suited for that.

> 

> Can you extend bridge command to allow setting/getting these bridge attrs?

> Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No changes

> needed to the kernel.

> 

> bridge link set dev br0 ageing_time 1000

> 

>  --or--

> 

> ip link set dev br0 type bridge ageing_time 1000


Being able to set these attributes via both bridge and ip would be great.

> >> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time) {

> >> +     struct switchdev_attr attr = {

> >> +             .id = SWITCHDEV_ATTR_BRIDGE,

> >> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,

> >> +             .u.bridge.attr = IFLA_BR_AGEING_TIME,

> >> +             .u.bridge.val = ageing_time,

> >> +     };

> >> +     int err;

> >> +

> >> +     err = switchdev_port_attr_set(br->dev, &attr);

> >> +     if (err)

> >> +             return err;

> >> +

> >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);

> >

> > Should we restart the timer here the new time takes effect?

> 

> I don't know...I just copied what the original code did.  If it does need to be

> restarted, break that out as a separate patch.


In my opinion, yes, the timer should be restarted. If the timer had been set to 1 million seconds and is being changed to 1 minute, you wouldn't want to wait for the 1-million-second  timer to expire before resetting it to the newly-configured 1-minute timer value.

Dan.

Thank you Scott.  Please see inline.

> >> -----Original Message-----

> >> From: Scott Feldman [mailto:sfeldma@gmail.com]

> >> Sent: Tuesday, August 18, 2015 12:48 PM

> >> To: Premkumar Jonnala

> >> Cc: netdev@vger.kernel.org

> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for

> bridges

> >> and switch devices.

> >>

> >>

> >>

> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:

> >>

> >> > Bridge devices have ageing interval used to age out MAC addresses

> >> > from FDB.  This ageing interval was not configuratble.

> >> >

> >> > Enable netlink based configuration of ageing interval for bridges and

> >> > switch devices.  The ageing interval changes the timer used to purge

> >> > inactive FDB entries in bridges.  The ageing interval config is

> >> > propagated to switch devices, so that platform or hardware based

> >> > ageing works according to configuration.

> >> >

> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>

> >>

> >> Hi Premkumar,

> >>

> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.

> >

> > What is the motivation for using 'ip link' command to configure bridge

> attributes?  IMHO,

> > bridge command is better suited for that.

> 

> Can you extend bridge command to allow setting/getting these bridge

> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No

> changes needed to the kernel.

> 

> bridge link set dev br0 ageing_time 1000

> 

>  --or--

> 

> ip link set dev br0 type bridge ageing_time 1000


I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and move them to 'bridge' command.  

> 

> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c

> >> index 0f2408f..01401ea 100644

> >> --- a/net/bridge/br_netlink.c

> >> +++ b/net/bridge/br_netlink.c

> >> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev,

> struct

> >> nlattr *tb[],

> >>       }

> >>

> >>       if (data[IFLA_BR_AGEING_TIME]) {

> >> -             u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);

> >

> > Should we do some range checking here to ensure that the value is within a

> certain range.

> > IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million

> seconds.

> 

> Sure, but make that a separate patch.


Sure.

> 

> >> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)

> >> +{

> >> +     struct switchdev_attr attr = {

> >> +             .id = SWITCHDEV_ATTR_BRIDGE,

> >> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,

> >> +             .u.bridge.attr = IFLA_BR_AGEING_TIME,

> >> +             .u.bridge.val = ageing_time,

> >> +     };

> >> +     int err;

> >> +

> >> +     err = switchdev_port_attr_set(br->dev, &attr);

> >> +     if (err)

> >> +             return err;

> >> +

> >> +     br->ageing_time = clock_t_to_jiffies(ageing_time);

> >

> > Should we restart the timer here the new time takes effect?

> 

> I don't know...I just copied what the original code did.  If it does

> need to be restarted, break that out as a separate patch.


There is another comment on this thread where it was suggested that we restart the timer.  I will
make the change in a separate patch.

-Prem

> 

> -scott

On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
> Thank you Scott.  Please see inline.
>
>> >> -----Original Message-----
>> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> >> Sent: Tuesday, August 18, 2015 12:48 PM
>> >> To: Premkumar Jonnala
>> >> Cc: netdev@vger.kernel.org
>> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
>> bridges
>> >> and switch devices.
>> >>
>> >>
>> >>
>> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>> >>
>> >> > Bridge devices have ageing interval used to age out MAC addresses
>> >> > from FDB.  This ageing interval was not configuratble.
>> >> >
>> >> > Enable netlink based configuration of ageing interval for bridges and
>> >> > switch devices.  The ageing interval changes the timer used to purge
>> >> > inactive FDB entries in bridges.  The ageing interval config is
>> >> > propagated to switch devices, so that platform or hardware based
>> >> > ageing works according to configuration.
>> >> >
>> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>> >>
>> >> Hi Premkumar,
>> >>
>> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>> >
>> > What is the motivation for using 'ip link' command to configure bridge
>> attributes?  IMHO,
>> > bridge command is better suited for that.
>>
>> Can you extend bridge command to allow setting/getting these bridge
>> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
>> changes needed to the kernel.
>>
>> bridge link set dev br0 ageing_time 1000
>>
>>  --or--
>>
>> ip link set dev br0 type bridge ageing_time 1000
>
> I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and move them to 'bridge' command.

We're probably stuck with the 'ip link' commands, since they're
already release in the wild and folks may have dependency on them.
However, when looking at iproute2 code, look for opportunity to use
same code for both paths.

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

On Wed, Aug 19, 2015 at 10:12 PM, Premkumar Jonnala
<pjonnala@broadcom.com> wrote:
>
>
>> -----Original Message-----
>> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> Sent: Thursday, August 20, 2015 10:31 AM
>> To: Premkumar Jonnala
>> Cc: netdev@vger.kernel.org
>> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
>> and switch devices.
>>
>> On Wed, Aug 19, 2015 at 9:56 PM, Premkumar Jonnala
>> <pjonnala@broadcom.com> wrote:
>> > Thank you Scott.  Please see inline.
>> >
>> >> >> -----Original Message-----
>> >> >> From: Scott Feldman [mailto:sfeldma@gmail.com]
>> >> >> Sent: Tuesday, August 18, 2015 12:48 PM
>> >> >> To: Premkumar Jonnala
>> >> >> Cc: netdev@vger.kernel.org
>> >> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
>> >> bridges
>> >> >> and switch devices.
>> >> >>
>> >> >>
>> >> >>
>> >> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>> >> >>
>> >> >> > Bridge devices have ageing interval used to age out MAC addresses
>> >> >> > from FDB.  This ageing interval was not configuratble.
>> >> >> >
>> >> >> > Enable netlink based configuration of ageing interval for bridges and
>> >> >> > switch devices.  The ageing interval changes the timer used to purge
>> >> >> > inactive FDB entries in bridges.  The ageing interval config is
>> >> >> > propagated to switch devices, so that platform or hardware based
>> >> >> > ageing works according to configuration.
>> >> >> >
>> >> >> > Signed-off-by: Premkumar Jonnala <pjonnala@broadcom.com>
>> >> >>
>> >> >> Hi Premkumar,
>> >> >>
>> >> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
>> >> >
>> >> > What is the motivation for using 'ip link' command to configure bridge
>> >> attributes?  IMHO,
>> >> > bridge command is better suited for that.
>> >>
>> >> Can you extend bridge command to allow setting/getting these bridge
>> >> attrs?  Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
>> >> changes needed to the kernel.
>> >>
>> >> bridge link set dev br0 ageing_time 1000
>> >>
>> >>  --or--
>> >>
>> >> ip link set dev br0 type bridge ageing_time 1000
>> >
>> > I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and
>> move them to 'bridge' command.
>>
>> We're probably stuck with the 'ip link' commands, since they're
>> already release in the wild and folks may have dependency on them.
>> However, when looking at iproute2 code, look for opportunity to use
>> same code for both paths.
>
> Ok.  Then we can have both ip and bridge commands supporting these options, and freeze the 'ip link' command as it
> exists today.  Any new options in future should be added to the bridge command.  Does that sound okay?

It would be ideal if both command paths use same code so new options
work with either command.  There are other examples where shared code
approach could be used to create synonymous commands:

ip link add name br0 type bridge       --or-- bridge add dev br0
ip link del br0                                  --or-- bridge del dev br0
ip link set dev sw1p1 master br0       --or-- bridge link add dev sw1p1 br0
ip link set dev sw1p1 nomaster br0    --or-- bridge link del sw1p1

There is some precedence of synonymous bridge commands:

ip link set dev sw1p1 type bridge_slave flood on
--or--
bridge link set dev sw1p1 flood on

But I don't know if the same code path is used for both of these.  (It should).

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

On Thu, Aug 20, 2015 at 05:08:51AM +0000, Premkumar Jonnala wrote:
> > From: Wilson, Daniel G [mailto:daniel.wilson@intel.com]
> > >
> > > Can you extend bridge command to allow setting/getting these bridge attrs?
> > > Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No changes
> > > needed to the kernel.
> > >
> > > bridge link set dev br0 ageing_time 1000
> > >
> > >  --or--
> > >
> > > ip link set dev br0 type bridge ageing_time 1000
> > 
> > Being able to set these attributes via both bridge and ip would be great.
> > 
> IMHO, we should choose only one command.  Otherwise, we'd have to
> spend effort in trying to keep both the commands in sync.

As long as they are using the same netlink interface, I don't think it's
a serious problem. After all, there will be also other tools (wicked,
perhaps systemd-networkd) setting it directly via netlink rather than
calling either ip or bridge.

> My vote would be for the bridge command - since the options/parameters
> are related to bridges.  If there is no objection, I'll move all the
> bridge options from 'ip link' command to 'bridge' command.

This would break existing scripts using ip to set the parameter. Is the
possibility to use any of the two really that bad?

                                                         Michal Kubecek

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

> -----Original Message-----
> From: Michal Kubecek [mailto:mkubecek@suse.cz]
> Sent: Thursday, August 20, 2015 12:00 PM
> To: Premkumar Jonnala
> Cc: Wilson, Daniel G; Scott Feldman; netdev@vger.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
> 
> On Thu, Aug 20, 2015 at 05:08:51AM +0000, Premkumar Jonnala wrote:
> > > From: Wilson, Daniel G [mailto:daniel.wilson@intel.com]
> > > >
> > > > Can you extend bridge command to allow setting/getting these bridge
> attrs?
> > > > Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg.  No
> changes
> > > > needed to the kernel.
> > > >
> > > > bridge link set dev br0 ageing_time 1000
> > > >
> > > >  --or--
> > > >
> > > > ip link set dev br0 type bridge ageing_time 1000
> > >
> > > Being able to set these attributes via both bridge and ip would be great.
> > >
> > IMHO, we should choose only one command.  Otherwise, we'd have to
> > spend effort in trying to keep both the commands in sync.
> 
> As long as they are using the same netlink interface, I don't think it's
> a serious problem. After all, there will be also other tools (wicked,
> perhaps systemd-networkd) setting it directly via netlink rather than
> calling either ip or bridge.
> 
> > My vote would be for the bridge command - since the options/parameters
> > are related to bridges.  If there is no objection, I'll move all the
> > bridge options from 'ip link' command to 'bridge' command.
> 
> This would break existing scripts using ip to set the parameter. Is the
> possibility to use any of the two really that bad?

There was another email on this thread where Scott indicated existence of other commands
where both ip and bridge are available, and they are for the same function.

I will keep both the ip and bridge commands, and try to share the underlying code as much as possible.

-Prem

> 
>                                                          Michal Kubecek

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

On Thu, Aug 20, 2015 at 06:40:01AM +0000, Premkumar Jonnala wrote:
> > From: Michal Kubecek [mailto:mkubecek@suse.cz]
> > 
> > This would break existing scripts using ip to set the parameter. Is the
> > possibility to use any of the two really that bad?
> 
> There was another email on this thread where Scott indicated existence
> of other commands where both ip and bridge are available, and they are
> for the same function.

Yes, I already noticed it. I'm sorry, I should have checked the whole
thread before replying.

                                                       Michal Kubecek

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