[PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

This patch allows user to configure bridge's FDB ageing using
netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
as per ieee8021QBridgeFdbAgingTime.

Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
---
Changes in v2:
 - Added commit message.
 - Fix style problems reported by checkpatch.pl.

 include/uapi/linux/if_link.h |  1 +
 net/bridge/br_device.c       | 13 +++++++++++++
 net/bridge/br_netlink.c      | 14 ++++++++++++--
 net/bridge/br_private.h      |  5 +++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 756436e..4a59232 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -224,6 +224,7 @@ enum {
 	IFLA_BR_FORWARD_DELAY,
 	IFLA_BR_HELLO_TIME,
 	IFLA_BR_MAX_AGE,
+	IFLA_BR_AGEING_TIME,
 	__IFLA_BR_MAX,
 };
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 294cbcc..1a665aa 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -392,3 +392,16 @@ void br_dev_setup(struct net_device *dev)
 	br_stp_timer_init(br);
 	br_multicast_init(br);
 }
+
+int br_set_ageing_time(struct net_bridge *br, unsigned long val)
+{
+	unsigned long t = clock_t_to_jiffies(val);
+
+	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
+		return -ERANGE;
+
+	spin_lock_bh(&br->lock);
+	br->ageing_time = t;
+	spin_unlock_bh(&br->lock);
+	return 0;
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 8bc6b67..d80e802 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -733,6 +733,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
 	[IFLA_BR_FORWARD_DELAY]	= { .type = NLA_U32 },
 	[IFLA_BR_HELLO_TIME]	= { .type = NLA_U32 },
 	[IFLA_BR_MAX_AGE]	= { .type = NLA_U32 },
+	[IFLA_BR_AGEING_TIME]	= { .type = NLA_U32 },
 };
 
 static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
@@ -762,6 +763,12 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			return err;
 	}
 
+	if (data[IFLA_BR_AGEING_TIME]) {
+		err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
@@ -770,6 +777,7 @@ static size_t br_get_size(const struct net_device *brdev)
 	return nla_total_size(sizeof(u32)) +	/* IFLA_BR_FORWARD_DELAY  */
 	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_HELLO_TIME */
 	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_MAX_AGE */
+	       nla_total_size(sizeof(u32)) +	/* IFLA_BR_AGEING_TIME */
 	       0;
 }
 
@@ -778,11 +786,13 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	struct net_bridge *br = netdev_priv(brdev);
 	u32 forward_delay = jiffies_to_clock_t(br->forward_delay);
 	u32 hello_time = jiffies_to_clock_t(br->hello_time);
-	u32 age_time = jiffies_to_clock_t(br->max_age);
+	u32 max_age = jiffies_to_clock_t(br->max_age);
+	u32 ageing_time = jiffies_to_clock_t(br->ageing_time);
 
 	if (nla_put_u32(skb, IFLA_BR_FORWARD_DELAY, forward_delay) ||
 	    nla_put_u32(skb, IFLA_BR_HELLO_TIME, hello_time) ||
-	    nla_put_u32(skb, IFLA_BR_MAX_AGE, age_time))
+	    nla_put_u32(skb, IFLA_BR_MAX_AGE, max_age) ||
+	    nla_put_u32(skb, IFLA_BR_AGEING_TIME, ageing_time))
 		return -EMSGSIZE;
 
 	return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f0a0438..325883c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -25,6 +25,10 @@
 
 #define BR_HOLD_TIME (1*HZ)
 
+/* values as per ieee8021QBridgeFdbAgingTime */
+#define BR_MIN_AGEING_TIME  (10 * HZ)
+#define BR_MAX_AGEING_TIME  (1000000 * HZ)
+
 #define BR_PORT_BITS	10
 #define BR_MAX_PORTS	(1<<BR_PORT_BITS)
 #define BR_VLAN_BITMAP_LEN	BITS_TO_LONGS(VLAN_N_VID)
@@ -345,6 +349,7 @@ static inline int br_is_root_bridge(const struct net_bridge *br)
 void br_dev_setup(struct net_device *dev);
 void br_dev_delete(struct net_device *dev, struct list_head *list);
 netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev);
+int br_set_ageing_time(struct net_bridge *br, unsigned long val);
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
 				       struct sk_buff *skb)
-- 
2.1.0

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Scott Feldman]

On Fri, Mar 13, 2015 at 6:40 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
> This patch allows user to configure bridge's FDB ageing using
> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
> as per ieee8021QBridgeFdbAgingTime.
>
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>

Reviewed-by: Scott Feldman <sfeldma@gmail.com>

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Scott Feldman]

On Fri, Mar 13, 2015 at 6:40 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
> This patch allows user to configure bridge's FDB ageing using
> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
> as per ieee8021QBridgeFdbAgingTime.
>
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> ---
> Changes in v2:
>  - Added commit message.
>  - Fix style problems reported by checkpatch.pl.


> +
> +int br_set_ageing_time(struct net_bridge *br, unsigned long val)
> +{
> +       unsigned long t = clock_t_to_jiffies(val);
> +
> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> +               return -ERANGE;
> +
> +       spin_lock_bh(&br->lock);
> +       br->ageing_time = t;

I wonder if you need to call mod_timer(&br->gc_timer, jiffies) after
adjusting the time, to make new ageing_time effective?  The worst-case
scenario I'm thinking about is if the initial br->ageing_time is the
max value (1000000 seconds) and the user changes it the min value (10
seconds), will the original 1000000 seconds need to expire before the
gc_timer is called again and reset to 10 seconds?

> +       spin_unlock_bh(&br->lock);
> +       return 0;
> +}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 8bc6b67..d80e802 100644

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

On Sun, Mar 15, 2015 at 3:47 AM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Fri, Mar 13, 2015 at 6:40 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
>> This patch allows user to configure bridge's FDB ageing using
>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>> as per ieee8021QBridgeFdbAgingTime.
>>
>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>> ---
>> Changes in v2:
>>  - Added commit message.
>>  - Fix style problems reported by checkpatch.pl.
>
>
>> +
>> +int br_set_ageing_time(struct net_bridge *br, unsigned long val)
>> +{
>> +       unsigned long t = clock_t_to_jiffies(val);
>> +
>> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> +               return -ERANGE;
>> +
>> +       spin_lock_bh(&br->lock);
>> +       br->ageing_time = t;
>
> I wonder if you need to call mod_timer(&br->gc_timer, jiffies) after
> adjusting the time, to make new ageing_time effective?  The worst-case
> scenario I'm thinking about is if the initial br->ageing_time is the
> max value (1000000 seconds) and the user changes it the min value (10
> seconds), will the original 1000000 seconds need to expire before the
> gc_timer is called again and reset to 10 seconds?
>
Yes, the new ageing_time becomes effective only after the current timer expires.
The behavior when ageing_time is set via netlink  is similar to setting it using
sysfs and brctl. I want to keep the behavior same when the ageing_time is
configured using any of the existing mechanisms.

If this needs to become effective immediately, I will work on it via
another patch/RFC.

>> +       spin_unlock_bh(&br->lock);
>> +       return 0;
>> +}
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 8bc6b67..d80e802 100644



-- 
Regards,
Siva Mannem.

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Scott Feldman]

On Sat, Mar 14, 2015 at 4:36 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
> On Sun, Mar 15, 2015 at 3:47 AM, Scott Feldman <sfeldma@gmail.com> wrote:
>> On Fri, Mar 13, 2015 at 6:40 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
>>> This patch allows user to configure bridge's FDB ageing using
>>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>>> as per ieee8021QBridgeFdbAgingTime.
>>>
>>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>>> ---
>>> Changes in v2:
>>>  - Added commit message.
>>>  - Fix style problems reported by checkpatch.pl.
>>
>>
>>> +
>>> +int br_set_ageing_time(struct net_bridge *br, unsigned long val)
>>> +{
>>> +       unsigned long t = clock_t_to_jiffies(val);
>>> +
>>> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>> +               return -ERANGE;
>>> +
>>> +       spin_lock_bh(&br->lock);
>>> +       br->ageing_time = t;
>>
>> I wonder if you need to call mod_timer(&br->gc_timer, jiffies) after
>> adjusting the time, to make new ageing_time effective?  The worst-case
>> scenario I'm thinking about is if the initial br->ageing_time is the
>> max value (1000000 seconds) and the user changes it the min value (10
>> seconds), will the original 1000000 seconds need to expire before the
>> gc_timer is called again and reset to 10 seconds?
>>
> Yes, the new ageing_time becomes effective only after the current timer expires.
> The behavior when ageing_time is set via netlink  is similar to setting it using
> sysfs and brctl. I want to keep the behavior same when the ageing_time is
> configured using any of the existing mechanisms.

Right, looks like sysfs and brctl (ioctl) don't reset gc_timer, so
you're consistent there.  :)

Consistency is good.  What do you think about a new func that's called
from either of the three interfaces (sysfs, ioctl, netlink) that:

1) validates range, like in your br_set_ageing_time() func.
2) reset gc_timer if setting ageing_time to lower value

?

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

On Sun, Mar 15, 2015 at 6:46 AM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Sat, Mar 14, 2015 at 4:36 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
>> On Sun, Mar 15, 2015 at 3:47 AM, Scott Feldman <sfeldma@gmail.com> wrote:
>>> On Fri, Mar 13, 2015 at 6:40 PM, Siva Mannem <siva.mannem.lnx@gmail.com> wrote:
>>>> This patch allows user to configure bridge's FDB ageing using
>>>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>>>> as per ieee8021QBridgeFdbAgingTime.
>>>>
>>>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>  - Added commit message.
>>>>  - Fix style problems reported by checkpatch.pl.
>>>
>>>
>>>> +
>>>> +int br_set_ageing_time(struct net_bridge *br, unsigned long val)
>>>> +{
>>>> +       unsigned long t = clock_t_to_jiffies(val);
>>>> +
>>>> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>>> +               return -ERANGE;
>>>> +
>>>> +       spin_lock_bh(&br->lock);
>>>> +       br->ageing_time = t;
>>>
>>> I wonder if you need to call mod_timer(&br->gc_timer, jiffies) after
>>> adjusting the time, to make new ageing_time effective?  The worst-case
>>> scenario I'm thinking about is if the initial br->ageing_time is the
>>> max value (1000000 seconds) and the user changes it the min value (10
>>> seconds), will the original 1000000 seconds need to expire before the
>>> gc_timer is called again and reset to 10 seconds?
>>>
>> Yes, the new ageing_time becomes effective only after the current timer expires.
>> The behavior when ageing_time is set via netlink  is similar to setting it using
>> sysfs and brctl. I want to keep the behavior same when the ageing_time is
>> configured using any of the existing mechanisms.
>
> Right, looks like sysfs and brctl (ioctl) don't reset gc_timer, so
> you're consistent there.  :)
>
> Consistency is good.  What do you think about a new func that's called
> from either of the three interfaces (sysfs, ioctl, netlink) that:
>
> 1) validates range, like in your br_set_ageing_time() func.
> 2) reset gc_timer if setting ageing_time to lower value
>
> ?
Agree. I will work on it.


-- 
Regards,
Siva Mannem.

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[David Miller]

From: Siva Mannem <siva.mannem.lnx@gmail.com>
Date: Sat, 14 Mar 2015 07:10:29 +0530

> This patch allows user to configure bridge's FDB ageing using
> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
> as per ieee8021QBridgeFdbAgingTime.
> 
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>

The behavior of br_changelink() leaves a lot to be desired, and this
change is making it worse.

The range of the netlink attributes, the only thing that can cause
an error, should be validated for all attributes _first_.

Because right now you can have several values change state,
then the last one has a range error, and an error is returned
without rolling back the state.

This is terrible.

When this happens the user has not reliable way to figure out
that some of the state changes it requested happened, and
exactly which ones those were.

If an error is thrown we _MUST_ not make any state changes to
the bridge whatsoever.

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

On Mon, Mar 16, 2015 at 7:43 AM, David Miller <davem@davemloft.net> wrote:
> From: Siva Mannem <siva.mannem.lnx@gmail.com>
> Date: Sat, 14 Mar 2015 07:10:29 +0530
>
>> This patch allows user to configure bridge's FDB ageing using
>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>> as per ieee8021QBridgeFdbAgingTime.
>>
>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>
> The behavior of br_changelink() leaves a lot to be desired, and this
> change is making it worse.
>
> The range of the netlink attributes, the only thing that can cause
> an error, should be validated for all attributes _first_.
>
> Because right now you can have several values change state,
> then the last one has a range error, and an error is returned
> without rolling back the state.
>
> This is terrible.
>
> When this happens the user has not reliable way to figure out
> that some of the state changes it requested happened, and
> exactly which ones those were.
>
> If an error is thrown we _MUST_ not make any state changes to
> the bridge whatsoever.

Agree. Will work on it. Can it be a follow-on patch?

-- 
Regards,
Siva Mannem.

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[Siva Mannem]

On Mon, Mar 16, 2015 at 7:43 AM, David Miller <davem@davemloft.net> wrote:
> From: Siva Mannem <siva.mannem.lnx@gmail.com>
> Date: Sat, 14 Mar 2015 07:10:29 +0530
>
>> This patch allows user to configure bridge's FDB ageing using
>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>> as per ieee8021QBridgeFdbAgingTime.
>>
>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>
> The behavior of br_changelink() leaves a lot to be desired, and this
> change is making it worse.
>
> The range of the netlink attributes, the only thing that can cause
> an error, should be validated for all attributes _first_.
>
I will check whether this can be achieved by adding the required range checks
to br_validate(). Any side-effects with this approach?

> Because right now you can have several values change state,
> then the last one has a range error, and an error is returned
> without rolling back the state.
>

> This is terrible.
>
> When this happens the user has not reliable way to figure out
> that some of the state changes it requested happened, and
> exactly which ones those were.
>
> If an error is thrown we _MUST_ not make any state changes to
> the bridge whatsoever.



-- 
Regards,
Siva Mannem.

Re: [PATCH net-next v2] Configure bridge FDB ageing time using netlink.

[David Miller]

From: Siva Mannem <siva.mannem.lnx@gmail.com>
Date: Mon, 16 Mar 2015 12:12:39 +0530

> On Mon, Mar 16, 2015 at 7:43 AM, David Miller <davem@davemloft.net> wrote:
>> From: Siva Mannem <siva.mannem.lnx@gmail.com>
>> Date: Sat, 14 Mar 2015 07:10:29 +0530
>>
>>> This patch allows user to configure bridge's FDB ageing using
>>> netlink(for ex, iproute2). Allowed range is 10 seconds to 1000000 seconds
>>> as per ieee8021QBridgeFdbAgingTime.
>>>
>>> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>>
>> The behavior of br_changelink() leaves a lot to be desired, and this
>> change is making it worse.
>>
>> The range of the netlink attributes, the only thing that can cause
>> an error, should be validated for all attributes _first_.
>>
>> Because right now you can have several values change state,
>> then the last one has a range error, and an error is returned
>> without rolling back the state.
>>
>> This is terrible.
>>
>> When this happens the user has not reliable way to figure out
>> that some of the state changes it requested happened, and
>> exactly which ones those were.
>>
>> If an error is thrown we _MUST_ not make any state changes to
>> the bridge whatsoever.
> 
> Agree. Will work on it. Can it be a follow-on patch?

No, I don't want to apply your change as-is which makes the situation
worse.