Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Andrew Lunn]

> OK. I think caching per-port (and thus per-bridge) ageing time would do
> the trick and keep DSA drivers simple. What about the following patch?

Hi Vivien

This looks good. I would suggest you do some testing with a printk,
and force some topology changes, just to make sure it works how we
think it works.

Andrew

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> On Mon, Jul 18, 2016 at 03:59:38PM -0400, Vivien Didelot wrote:
>> Andrew Lunn  writes:
>> 
>> >> Nope, the bridge ageing time is not per-port, even though switchdev ops
>> >> are per-port by design. This is a switch-wide attribute.
>> >
>> > So you are saying the core is doing all the reference counting, etc,
>> > when swapping between fast and slow ageing?
>> 
>> I don't see how checking for the fastest ageing time would fix support
>> for multiple bridges...
>
> The bridge should switch to fast ageing after a topology change to
> flush out entries which are now wrong. Using the short age time for
> too long results in a bit more inefficiency, in that entries time out
> faster than they need to. But if we go back to slow ageing too
> quickly, e.g. because of another bridge, we get wrong operation, in
> that bad entries can get stuck in the table for up to 5 minutes.
>
> So either we need to keep fast ageing as long as there is one bridge
> fast ageing, or we need to flush the whole MAC cache for a bridge on
> topology change and don't bother with fast ageing at all.
>
>> Maybe we can keep it simple for the moment with this switch-wide
>> set_ageing_time operation, and later add a patch for the DSA layer to
>> cache and elect the ageing time per-port or per-bridge.
>
> I don't think it can be done at the DSA layer. It does not have the
> information needed.

OK. I think caching per-port (and thus per-bridge) ageing time would do
the trick and keep DSA drivers simple. What about the following patch?

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 238fad9..2217a3f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -141,6 +141,7 @@ struct dsa_switch_tree {
 struct dsa_port {
struct net_device   *netdev;
struct device_node  *dn;
+   unsigned intageing_time;
 };
 
 struct dsa_switch {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1074cb6..fc91967 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -333,6 +333,21 @@ static int dsa_slave_vlan_filtering(struct net_device *dev,
return 0;
 }
 
+static int dsa_fastest_ageing_time(struct dsa_switch *ds,
+  unsigned int ageing_time)
+{
+   int i;
+
+   for (i = 0; i < DSA_MAX_PORTS; ++i) {
+   struct dsa_port *dp = >ports[i];
+
+   if (dp && dp->ageing_time && dp->ageing_time < ageing_time)
+   ageing_time = dp->ageing_time;
+   }
+
+   return ageing_time;
+}
+
 static int dsa_slave_ageing_time(struct net_device *dev,
 const struct switchdev_attr *attr,
 struct switchdev_trans *trans)
@@ -346,6 +361,10 @@ static int dsa_slave_ageing_time(struct net_device *dev,
if (switchdev_trans_ph_prepare(trans))
return 0;
 
+   /* Keep the fastest ageing time in case of multiple bridges */
+   ds->ports[p->port].ageing_time = ageing_time;
+   ageing_time = dsa_fastest_ageing_time(ds, ageing_time);
+
if (ds->drv->set_ageing_time)
return ds->drv->set_ageing_time(ds, ageing_time);
 

Thanks,

Vivien

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Andrew Lunn]

On Mon, Jul 18, 2016 at 03:59:38PM -0400, Vivien Didelot wrote:
> Andrew Lunn  writes:
> 
> >> Nope, the bridge ageing time is not per-port, even though switchdev ops
> >> are per-port by design. This is a switch-wide attribute.
> >
> > So you are saying the core is doing all the reference counting, etc,
> > when swapping between fast and slow ageing?
> 
> I don't see how checking for the fastest ageing time would fix support
> for multiple bridges...

The bridge should switch to fast ageing after a topology change to
flush out entries which are now wrong. Using the short age time for
too long results in a bit more inefficiency, in that entries time out
faster than they need to. But if we go back to slow ageing too
quickly, e.g. because of another bridge, we get wrong operation, in
that bad entries can get stuck in the table for up to 5 minutes.

So either we need to keep fast ageing as long as there is one bridge
fast ageing, or we need to flush the whole MAC cache for a bridge on
topology change and don't bother with fast ageing at all.

> Maybe we can keep it simple for the moment with this switch-wide
> set_ageing_time operation, and later add a patch for the DSA layer to
> cache and elect the ageing time per-port or per-bridge.

I don't think it can be done at the DSA layer. It does not have the
information needed.

Andrew

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Vivien Didelot]

Andrew Lunn  writes:

>> Nope, the bridge ageing time is not per-port, even though switchdev ops
>> are per-port by design. This is a switch-wide attribute.
>
> So you are saying the core is doing all the reference counting, etc,
> when swapping between fast and slow ageing?

I don't see how checking for the fastest ageing time would fix support
for multiple bridges... I think that would make the code much more
complex for a small value. Multiple logical bridges on top of a single
physical switch is still a tricky topic.

Maybe we can keep it simple for the moment with this switch-wide
set_ageing_time operation, and later add a patch for the DSA layer to
cache and elect the ageing time per-port or per-bridge.

Thanks,

Vivien

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Andrew Lunn]

> Nope, the bridge ageing time is not per-port, even though switchdev ops
> are per-port by design. This is a switch-wide attribute.

So you are saying the core is doing all the reference counting, etc,
when swapping between fast and slow ageing?

 Andrew

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> This is way too simplistic.
>
> The switchdev call is per port. This manipulates the whole switch. We
> need to somehow handle the difference.
>
> I've not look at the bridge code, but i assume it initially sets each
> port to a long age time, probably 5 minutes. When there is a topology
> change, it enables fast ageing by setting a shorter age time in each
> port. After a while it will return to the default age time. Although
> the switchdev call is per port, i think the age time is a property of
> the bridge, not a port.
>
> For the Marvell devices, we only have a global setting. It will apply
> to all bridges we create on the switch. So if one bridge requests fast
> ageing, we need to apply it to all bridges. We should only go back to
> slow ageing when all bridges are out of fast ageing. That is, we need
> some sort of reference counting.
>
> I'm not sure we have enough information to know why the bridge changed
> the age timing. Did the use change the forwarding delay, or have we
> entered fast ageing? So i think for Marvell devices, we need an
> additional property passed down. Is this a fast or a slow age time?
> We can then determine what is the fastest fast ageing, and the fastest
> slow ageing is, perform reference counting as appropriate, and set the
> global setting as needed.

Nope, the bridge ageing time is not per-port, even though switchdev ops
are per-port by design. This is a switch-wide attribute.

See f55ac58ae64c ("switchdev: add bridge ageing_time attribute") [1]

Rocker and mlxsw implement AGEING_TIME switch-wide too.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=f55ac58ae64c

Thanks,

Vivien

Re: [PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Andrew Lunn]

On Mon, Jul 18, 2016 at 02:46:28PM -0400, Vivien Didelot wrote:
> Implement the DSA driver function to configure the bridge ageing time.
> 
> Signed-off-by: Vivien Didelot 
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c 
> b/drivers/net/dsa/mv88e6xxx/chip.c
> index e2627a8..2101241 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3002,6 +3002,19 @@ static int mv88e6xxx_g1_set_age_time(struct 
> mv88e6xxx_chip *chip,
>   return mv88e6xxx_write(chip, REG_GLOBAL, GLOBAL_ATU_CONTROL, val);
>  }
>  
> +static int mv88e6xxx_set_ageing_time(struct dsa_switch *ds,
> +  unsigned int ageing_time)
> +{
> + struct mv88e6xxx_chip *chip = ds_to_priv(ds);
> + int err;
> +
> + mutex_lock(>reg_lock);
> + err = mv88e6xxx_g1_set_age_time(chip, ageing_time);
> + mutex_unlock(>reg_lock);
> +
> + return err;
> +}
> +
>  static int mv88e6xxx_g1_setup(struct mv88e6xxx_chip *chip)
>  {
>   struct dsa_switch *ds = chip->ds;
> @@ -3985,6 +3998,7 @@ static struct dsa_switch_driver mv88e6xxx_switch_driver 
> = {
>   .set_eeprom = mv88e6xxx_set_eeprom,
>   .get_regs_len   = mv88e6xxx_get_regs_len,
>   .get_regs   = mv88e6xxx_get_regs,
> + .set_ageing_time= mv88e6xxx_set_ageing_time,
>   .port_bridge_join   = mv88e6xxx_port_bridge_join,
>   .port_bridge_leave  = mv88e6xxx_port_bridge_leave,
>   .port_stp_state_set = mv88e6xxx_port_stp_state_set,

Hi Vivien

This is way too simplistic.

The switchdev call is per port. This manipulates the whole switch. We
need to somehow handle the difference.

I've not look at the bridge code, but i assume it initially sets each
port to a long age time, probably 5 minutes. When there is a topology
change, it enables fast ageing by setting a shorter age time in each
port. After a while it will return to the default age time. Although
the switchdev call is per port, i think the age time is a property of
the bridge, not a port.

For the Marvell devices, we only have a global setting. It will apply
to all bridges we create on the switch. So if one bridge requests fast
ageing, we need to apply it to all bridges. We should only go back to
slow ageing when all bridges are out of fast ageing. That is, we need
some sort of reference counting.

I'm not sure we have enough information to know why the bridge changed
the age timing. Did the use change the forwarding delay, or have we
entered fast ageing? So i think for Marvell devices, we need an
additional property passed down. Is this a fast or a slow age time?
We can then determine what is the fastest fast ageing, and the fastest
slow ageing is, perform reference counting as appropriate, and set the
global setting as needed.

   Andrew

[PATCH v2 net-next v2 12/12] net: dsa: mv88e6xxx: add support for DSA ageing time

[Vivien Didelot]

Implement the DSA driver function to configure the bridge ageing time.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/chip.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index e2627a8..2101241 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3002,6 +3002,19 @@ static int mv88e6xxx_g1_set_age_time(struct 
mv88e6xxx_chip *chip,
return mv88e6xxx_write(chip, REG_GLOBAL, GLOBAL_ATU_CONTROL, val);
 }
 
+static int mv88e6xxx_set_ageing_time(struct dsa_switch *ds,
+unsigned int ageing_time)
+{
+   struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+   int err;
+
+   mutex_lock(>reg_lock);
+   err = mv88e6xxx_g1_set_age_time(chip, ageing_time);
+   mutex_unlock(>reg_lock);
+
+   return err;
+}
+
 static int mv88e6xxx_g1_setup(struct mv88e6xxx_chip *chip)
 {
struct dsa_switch *ds = chip->ds;
@@ -3985,6 +3998,7 @@ static struct dsa_switch_driver mv88e6xxx_switch_driver = 
{
.set_eeprom = mv88e6xxx_set_eeprom,
.get_regs_len   = mv88e6xxx_get_regs_len,
.get_regs   = mv88e6xxx_get_regs,
+   .set_ageing_time= mv88e6xxx_set_ageing_time,
.port_bridge_join   = mv88e6xxx_port_bridge_join,
.port_bridge_leave  = mv88e6xxx_port_bridge_leave,
.port_stp_state_set = mv88e6xxx_port_stp_state_set,
-- 
2.9.0