> -----Original Message----- > From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@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 > <pjonn...@broadcom.com> wrote: > > Hello Scott, > > > > Thank you for the diff and comments. Please see my comments inline. > > > >> -----Original Message----- > >> From: Scott Feldman [mailto:sfel...@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 <pjonn...@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.