Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Sat, Oct 10, 2015 at 8:56 AM, Vivien Didelot wrote: > Scott, didn't you have a plan to add a struct device for the parent of > switchdev ports? I had sent out a rough RFC for a switch device in the last window. I have continued working on it, and I plan to send it very soon, probably again as an RFC. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Sat, Oct 10, 2015 at 12:04 AM, Jiri Pirko wrote:
> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
>>On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
>>wrote:
>>>
>>>
-Original Message-
From: sfel...@gmail.com [mailto:sfel...@gmail.com]
Sent: Friday, October 09, 2015 7:53 AM
To: netdev@vger.kernel.org
Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
Premkumar Jonnala; step...@networkplumber.org;
ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
vivien.dide...@savoirfairelinux.com
Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time
down
to switchdev
From: Scott Feldman
Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
support setting ageing_time (or setting bridge attrs in general).
If push fails, don't update ageing_time in bridge and return err to user.
If push succeeds, update ageing_time in bridge and run gc_timer now to
recalabrate when to run gc_timer next, based on new ageing_time.
Signed-off-by: Scott Feldman
Signed-off-by: Jiri Pirko
>>
>>
>>
+int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
+{
+ struct switchdev_attr attr = {
+ .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
+ .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
+ .u.ageing_time = ageing_time,
+ };
+ unsigned long t = clock_t_to_jiffies(ageing_time);
+ int err;
+
+ if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
+ return -ERANGE;
+
+ err = switchdev_port_attr_set(br->dev, &attr);
>>>
>>> A thought - given that the ageing time is not a per-bridge-port attr, why
>>> are we using a "port based api"
>>> to pass the attribute down? May be I'm missing something here?
>>
>>I think Florian raised the same point earlier. Sigh, I think this
>>should be addressedv4 coming soon...thanks guys for keeping the
>>standard high.
>
> Scott, can you tell us how do you want to address this? I like the
> current implementation.
I like it also, but there is some awkwardness in calling
switchdev_port_attr_set() with the first argument being the bridge
netdev, not a port netdev. But, the basic algorithm to recurse from
_this_ netdev down to its lower netdevs works great in this case; it's
just the name "switchdev_port_attr_set" implies a port netdev for
top-level netdev. So I was thinking about adding another call,
something like "switchdev_master_attr_set", which basically just does
the same thing as switchdev_port_attr_set, except maybe skips the
check to call the ops->switchdev_port_attr_add on the top-level
netdev. But now I don't like that idea so much as "master" would be
confusing when your passing a bond netdev (which is also a master),
but the bond _is_ the port netdev this time, a port on the bridge.
So let's scrap v4 and go with v3. I think I can live with this naming
awkwardness, given that we got something for essentially free by using
switchdev_port_attr_set() in a new way.
Davem, I think we're OK going with v3.
(There is a follow-on discussion about a switch device, which we'll
continue but it shouldn't block this v3 version).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
> 2015-10-10 15:41 GMT-07:00 Vivien Didelot
> :
> > On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
> >> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com
> wrote:
> >> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> >> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
> >> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
> wrote:
> >> >> >>
> >> >> >>
> >> >> >>> -Original Message-
> >> >> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
> >> >> >>> Sent: Friday, October 09, 2015 7:53 AM
> >> >> >>> To: netdev@vger.kernel.org
> >> >> >>> Cc: da...@davemloft.net; j...@resnulli.us;
> siva.mannem@gmail.com;
> >> >> >>> Premkumar Jonnala; step...@networkplumber.org;
> >> >> >>> ro...@cumulusnetworks.com; and...@lunn.ch;
> f.faine...@gmail.com;
> >> >> >>> vivien.dide...@savoirfairelinux.com
> >> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting
> ageing_time down
> >> >> >>> to switchdev
> >> >> >>>
> >> >> >>> From: Scott Feldman
> >> >> >>>
> >> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge
> that don't
> >> >> >>> support setting ageing_time (or setting bridge attrs in general).
> >> >> >>>
> >> >> >>> If push fails, don't update ageing_time in bridge and return err to
> >> >> >>> user.
> >> >> >>>
> >> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now
> to
> >> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >> >> >>>
> >> >> >>> Signed-off-by: Scott Feldman
> >> >> >>> Signed-off-by: Jiri Pirko
> >> >> >
> >> >> >
> >> >> >
> >> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> >> >>> +{
> >> >> >>> + struct switchdev_attr attr = {
> >> >> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >> >> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> >> >>> + .u.ageing_time = ageing_time,
> >> >> >>> + };
> >> >> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
> >> >> >>> + int err;
> >> >> >>> +
> >> >> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >> >> >>> + return -ERANGE;
> >> >> >>> +
> >> >> >>> + err = switchdev_port_attr_set(br->dev, &attr);
> >> >> >>
> >> >> >> A thought - given that the ageing time is not a per-bridge-port
> >> >> >> attr, why
> are we using a "port based api"
> >> >> >> to pass the attribute down? May be I'm missing something here?
> >> >> >
> >> >> >I think Florian raised the same point earlier. Sigh, I think this
> >> >> >should be addressedv4 coming soon...thanks guys for keeping the
> >> >> >standard high.
> >> >>
> >> >> Scott, can you tell us how do you want to address this? I like the
> >> >> current implementation.
> >> >
> >> >Scott, didn't you have a plan to add a struct device for the parent of
> >> >switchdev ports?
> >> >
> >> >I think it would be good to introduce such device with an helper to
> >> >retrieve this upper parent, and move the switchdev ops to this guy.
> >> >
> >> >So switchdev drivers may implement such API calls:
> >> >
> >> >.obj_add(struct device *swdev, struct switchdev_obj *obj);
> >> >
> >> >.port_obj_add(struct device *swdev, struct net_device *port,
> >> > struct switchdev_obj *obj);
> >> >
> >> >Then switchdev code may have a parent API and the current port API may
> >> >look like this:
> >> >
> >> >int switchdev_port_obj_add(struct net_device *dev,
> >> > struct switchdev_obj *obj)
> >> >{
> >> >struct device *swdev = switchdev_get_parent(dev);
> >> >int err = -EOPNOTSUPP;
> >> >
> >> >if (swdev && swdev->switchdev_ops &&
> >> >swdev->switchdev_ops->port_obj_add)
> >> >err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
> >> >
> >> >return err;
> >> >}
> >>
> >> Fro the record, I don't see any reason for this "device". It would just
> >> introduce unnecessary complexicity. So far, we are fine without it.
> >
> > I wouldn't say that. I beleive that an Ethernet switch deserves its
> > struct device in the tree, since it is a physical chip, like any other.
>
> Agreed, but gating these patches because we do not yet have a device
> driver model for an Ethernet switch outside of its individual ports
> does not seem like it hurts the current patch series, nor the existing
> model (and future).
I don't think we have to gate this patch because of this change, but I wanted to
raise this issue because it didn't seem right to use "port level" API when what
we
want to configure is really a "bridge level" attribute.
>
> >
> > Configuring it through one of its port (net_device) is fine, since you
> > want to change the port behavior, and Linux config is on per-port basis.
> >
> > But the complexity is already introduced in the struct net_device with
> > the switchdev_ops. These ops really belong to the parent device, not to
> >
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Oct. Saturday 10 (41) 05:07 PM, Florian Fainelli wrote:
> 2015-10-10 15:41 GMT-07:00 Vivien Didelot
> :
> > On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
> >> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com
> >> wrote:
> >> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> >> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
> >> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
> >> >> > wrote:
> >> >> >>
> >> >> >>
> >> >> >>> -Original Message-
> >> >> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
> >> >> >>> Sent: Friday, October 09, 2015 7:53 AM
> >> >> >>> To: netdev@vger.kernel.org
> >> >> >>> Cc: da...@davemloft.net; j...@resnulli.us;
> >> >> >>> siva.mannem@gmail.com;
> >> >> >>> Premkumar Jonnala; step...@networkplumber.org;
> >> >> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
> >> >> >>> vivien.dide...@savoirfairelinux.com
> >> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting
> >> >> >>> ageing_time down
> >> >> >>> to switchdev
> >> >> >>>
> >> >> >>> From: Scott Feldman
> >> >> >>>
> >> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that
> >> >> >>> don't
> >> >> >>> support setting ageing_time (or setting bridge attrs in general).
> >> >> >>>
> >> >> >>> If push fails, don't update ageing_time in bridge and return err to
> >> >> >>> user.
> >> >> >>>
> >> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now
> >> >> >>> to
> >> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >> >> >>>
> >> >> >>> Signed-off-by: Scott Feldman
> >> >> >>> Signed-off-by: Jiri Pirko
> >> >> >
> >> >> >
> >> >> >
> >> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> >> >>> +{
> >> >> >>> + struct switchdev_attr attr = {
> >> >> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >> >> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> >> >>> + .u.ageing_time = ageing_time,
> >> >> >>> + };
> >> >> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
> >> >> >>> + int err;
> >> >> >>> +
> >> >> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >> >> >>> + return -ERANGE;
> >> >> >>> +
> >> >> >>> + err = switchdev_port_attr_set(br->dev, &attr);
> >> >> >>
> >> >> >> A thought - given that the ageing time is not a per-bridge-port
> >> >> >> attr, why are we using a "port based api"
> >> >> >> to pass the attribute down? May be I'm missing something here?
> >> >> >
> >> >> >I think Florian raised the same point earlier. Sigh, I think this
> >> >> >should be addressedv4 coming soon...thanks guys for keeping the
> >> >> >standard high.
> >> >>
> >> >> Scott, can you tell us how do you want to address this? I like the
> >> >> current implementation.
> >> >
> >> >Scott, didn't you have a plan to add a struct device for the parent of
> >> >switchdev ports?
> >> >
> >> >I think it would be good to introduce such device with an helper to
> >> >retrieve this upper parent, and move the switchdev ops to this guy.
> >> >
> >> >So switchdev drivers may implement such API calls:
> >> >
> >> >.obj_add(struct device *swdev, struct switchdev_obj *obj);
> >> >
> >> >.port_obj_add(struct device *swdev, struct net_device *port,
> >> > struct switchdev_obj *obj);
> >> >
> >> >Then switchdev code may have a parent API and the current port API may
> >> >look like this:
> >> >
> >> >int switchdev_port_obj_add(struct net_device *dev,
> >> > struct switchdev_obj *obj)
> >> >{
> >> >struct device *swdev = switchdev_get_parent(dev);
> >> >int err = -EOPNOTSUPP;
> >> >
> >> >if (swdev && swdev->switchdev_ops &&
> >> >swdev->switchdev_ops->port_obj_add)
> >> >err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
> >> >
> >> >return err;
> >> >}
> >>
> >> Fro the record, I don't see any reason for this "device". It would just
> >> introduce unnecessary complexicity. So far, we are fine without it.
> >
> > I wouldn't say that. I beleive that an Ethernet switch deserves its
> > struct device in the tree, since it is a physical chip, like any other.
>
> Agreed, but gating these patches because we do not yet have a device
> driver model for an Ethernet switch outside of its individual ports
> does not seem like it hurts the current patch series, nor the existing
> model (and future).
Sure, I didn't mean to NAK the patch with this comment, I just wrote it
because we raised a concern about an API higher than the port level.
> >
> > Configuring it through one of its port (net_device) is fine, since you
> > want to change the port behavior, and Linux config is on per-port basis.
> >
> > But the complexity is already introduced in the struct net_device with
> > the switchdev_ops. These ops really belong to t
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
2015-10-10 15:41 GMT-07:00 Vivien Didelot :
> On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
>> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com
>> wrote:
>> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
>> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
>> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
>> >> > wrote:
>> >> >>
>> >> >>
>> >> >>> -Original Message-
>> >> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
>> >> >>> Sent: Friday, October 09, 2015 7:53 AM
>> >> >>> To: netdev@vger.kernel.org
>> >> >>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
>> >> >>> Premkumar Jonnala; step...@networkplumber.org;
>> >> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
>> >> >>> vivien.dide...@savoirfairelinux.com
>> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting
>> >> >>> ageing_time down
>> >> >>> to switchdev
>> >> >>>
>> >> >>> From: Scott Feldman
>> >> >>>
>> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that
>> >> >>> don't
>> >> >>> support setting ageing_time (or setting bridge attrs in general).
>> >> >>>
>> >> >>> If push fails, don't update ageing_time in bridge and return err to
>> >> >>> user.
>> >> >>>
>> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
>> >> >>>
>> >> >>> Signed-off-by: Scott Feldman
>> >> >>> Signed-off-by: Jiri Pirko
>> >> >
>> >> >
>> >> >
>> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> >> >>> +{
>> >> >>> + struct switchdev_attr attr = {
>> >> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> >> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> >> >>> + .u.ageing_time = ageing_time,
>> >> >>> + };
>> >> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
>> >> >>> + int err;
>> >> >>> +
>> >> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> >> >>> + return -ERANGE;
>> >> >>> +
>> >> >>> + err = switchdev_port_attr_set(br->dev, &attr);
>> >> >>
>> >> >> A thought - given that the ageing time is not a per-bridge-port attr,
>> >> >> why are we using a "port based api"
>> >> >> to pass the attribute down? May be I'm missing something here?
>> >> >
>> >> >I think Florian raised the same point earlier. Sigh, I think this
>> >> >should be addressedv4 coming soon...thanks guys for keeping the
>> >> >standard high.
>> >>
>> >> Scott, can you tell us how do you want to address this? I like the
>> >> current implementation.
>> >
>> >Scott, didn't you have a plan to add a struct device for the parent of
>> >switchdev ports?
>> >
>> >I think it would be good to introduce such device with an helper to
>> >retrieve this upper parent, and move the switchdev ops to this guy.
>> >
>> >So switchdev drivers may implement such API calls:
>> >
>> >.obj_add(struct device *swdev, struct switchdev_obj *obj);
>> >
>> >.port_obj_add(struct device *swdev, struct net_device *port,
>> > struct switchdev_obj *obj);
>> >
>> >Then switchdev code may have a parent API and the current port API may
>> >look like this:
>> >
>> >int switchdev_port_obj_add(struct net_device *dev,
>> > struct switchdev_obj *obj)
>> >{
>> >struct device *swdev = switchdev_get_parent(dev);
>> >int err = -EOPNOTSUPP;
>> >
>> >if (swdev && swdev->switchdev_ops &&
>> >swdev->switchdev_ops->port_obj_add)
>> >err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
>> >
>> >return err;
>> >}
>>
>> Fro the record, I don't see any reason for this "device". It would just
>> introduce unnecessary complexicity. So far, we are fine without it.
>
> I wouldn't say that. I beleive that an Ethernet switch deserves its
> struct device in the tree, since it is a physical chip, like any other.
Agreed, but gating these patches because we do not yet have a device
driver model for an Ethernet switch outside of its individual ports
does not seem like it hurts the current patch series, nor the existing
model (and future).
>
> Configuring it through one of its port (net_device) is fine, since you
> want to change the port behavior, and Linux config is on per-port basis.
>
> But the complexity is already introduced in the struct net_device with
> the switchdev_ops. These ops really belong to the parent device, not to
> all of its ports.
I am not sure if complexity is the correct term here, bloat (to some
extent) maybe, since with what you are suggesting we could save one
set of function pointers per-port, and instead move that to a
global/switch-wide device implementing these operations. In essence,
there will be per-port switchdev_ops, bridge-specific, and maybe in
the future switch device specific.
>
> Ideally a switch device woul
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com
> wrote:
> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
> >> >wrote:
> >> >>
> >> >>
> >> >>> -Original Message-
> >> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
> >> >>> Sent: Friday, October 09, 2015 7:53 AM
> >> >>> To: netdev@vger.kernel.org
> >> >>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
> >> >>> Premkumar Jonnala; step...@networkplumber.org;
> >> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
> >> >>> vivien.dide...@savoirfairelinux.com
> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting
> >> >>> ageing_time down
> >> >>> to switchdev
> >> >>>
> >> >>> From: Scott Feldman
> >> >>>
> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> >> >>> support setting ageing_time (or setting bridge attrs in general).
> >> >>>
> >> >>> If push fails, don't update ageing_time in bridge and return err to
> >> >>> user.
> >> >>>
> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >> >>>
> >> >>> Signed-off-by: Scott Feldman
> >> >>> Signed-off-by: Jiri Pirko
> >> >
> >> >
> >> >
> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> >>> +{
> >> >>> + struct switchdev_attr attr = {
> >> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> >>> + .u.ageing_time = ageing_time,
> >> >>> + };
> >> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
> >> >>> + int err;
> >> >>> +
> >> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >> >>> + return -ERANGE;
> >> >>> +
> >> >>> + err = switchdev_port_attr_set(br->dev, &attr);
> >> >>
> >> >> A thought - given that the ageing time is not a per-bridge-port attr,
> >> >> why are we using a "port based api"
> >> >> to pass the attribute down? May be I'm missing something here?
> >> >
> >> >I think Florian raised the same point earlier. Sigh, I think this
> >> >should be addressedv4 coming soon...thanks guys for keeping the
> >> >standard high.
> >>
> >> Scott, can you tell us how do you want to address this? I like the
> >> current implementation.
> >
> >Scott, didn't you have a plan to add a struct device for the parent of
> >switchdev ports?
> >
> >I think it would be good to introduce such device with an helper to
> >retrieve this upper parent, and move the switchdev ops to this guy.
> >
> >So switchdev drivers may implement such API calls:
> >
> >.obj_add(struct device *swdev, struct switchdev_obj *obj);
> >
> >.port_obj_add(struct device *swdev, struct net_device *port,
> > struct switchdev_obj *obj);
> >
> >Then switchdev code may have a parent API and the current port API may
> >look like this:
> >
> >int switchdev_port_obj_add(struct net_device *dev,
> > struct switchdev_obj *obj)
> >{
> >struct device *swdev = switchdev_get_parent(dev);
> >int err = -EOPNOTSUPP;
> >
> >if (swdev && swdev->switchdev_ops &&
> >swdev->switchdev_ops->port_obj_add)
> >err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
> >
> >return err;
> >}
>
> Fro the record, I don't see any reason for this "device". It would just
> introduce unnecessary complexicity. So far, we are fine without it.
I wouldn't say that. I beleive that an Ethernet switch deserves its
struct device in the tree, since it is a physical chip, like any other.
Configuring it through one of its port (net_device) is fine, since you
want to change the port behavior, and Linux config is on per-port basis.
But the complexity is already introduced in the struct net_device with
the switchdev_ops. These ops really belong to the parent device, not to
all of its ports.
Ideally a switch device would be registered with this set of operations,
creates its net_devices, and will be accessible from a port net_device
through a netdev helper function.
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.dide...@savoirfairelinux.com wrote:
>On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
>> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
>> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
>> >wrote:
>> >>
>> >>
>> >>> -Original Message-
>> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
>> >>> Sent: Friday, October 09, 2015 7:53 AM
>> >>> To: netdev@vger.kernel.org
>> >>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
>> >>> Premkumar Jonnala; step...@networkplumber.org;
>> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
>> >>> vivien.dide...@savoirfairelinux.com
>> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time
>> >>> down
>> >>> to switchdev
>> >>>
>> >>> From: Scott Feldman
>> >>>
>> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> >>> support setting ageing_time (or setting bridge attrs in general).
>> >>>
>> >>> If push fails, don't update ageing_time in bridge and return err to user.
>> >>>
>> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> >>> recalabrate when to run gc_timer next, based on new ageing_time.
>> >>>
>> >>> Signed-off-by: Scott Feldman
>> >>> Signed-off-by: Jiri Pirko
>> >
>> >
>> >
>> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> >>> +{
>> >>> + struct switchdev_attr attr = {
>> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> >>> + .u.ageing_time = ageing_time,
>> >>> + };
>> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
>> >>> + int err;
>> >>> +
>> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> >>> + return -ERANGE;
>> >>> +
>> >>> + err = switchdev_port_attr_set(br->dev, &attr);
>> >>
>> >> A thought - given that the ageing time is not a per-bridge-port attr, why
>> >> are we using a "port based api"
>> >> to pass the attribute down? May be I'm missing something here?
>> >
>> >I think Florian raised the same point earlier. Sigh, I think this
>> >should be addressedv4 coming soon...thanks guys for keeping the
>> >standard high.
>>
>> Scott, can you tell us how do you want to address this? I like the
>> current implementation.
>
>Scott, didn't you have a plan to add a struct device for the parent of
>switchdev ports?
>
>I think it would be good to introduce such device with an helper to
>retrieve this upper parent, and move the switchdev ops to this guy.
>
>So switchdev drivers may implement such API calls:
>
>.obj_add(struct device *swdev, struct switchdev_obj *obj);
>
>.port_obj_add(struct device *swdev, struct net_device *port,
> struct switchdev_obj *obj);
>
>Then switchdev code may have a parent API and the current port API may
>look like this:
>
>int switchdev_port_obj_add(struct net_device *dev,
> struct switchdev_obj *obj)
>{
>struct device *swdev = switchdev_get_parent(dev);
>int err = -EOPNOTSUPP;
>
>if (swdev && swdev->switchdev_ops &&
>swdev->switchdev_ops->port_obj_add)
>err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
>
>return err;
>}
Fro the record, I don't see any reason for this "device". It would just
introduce unnecessary complexicity. So far, we are fine without it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
> >wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
> >>> Sent: Friday, October 09, 2015 7:53 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
> >>> Premkumar Jonnala; step...@networkplumber.org;
> >>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
> >>> vivien.dide...@savoirfairelinux.com
> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time
> >>> down
> >>> to switchdev
> >>>
> >>> From: Scott Feldman
> >>>
> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> >>> support setting ageing_time (or setting bridge attrs in general).
> >>>
> >>> If push fails, don't update ageing_time in bridge and return err to user.
> >>>
> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >>>
> >>> Signed-off-by: Scott Feldman
> >>> Signed-off-by: Jiri Pirko
> >
> >
> >
> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >>> +{
> >>> + struct switchdev_attr attr = {
> >>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >>> + .u.ageing_time = ageing_time,
> >>> + };
> >>> + unsigned long t = clock_t_to_jiffies(ageing_time);
> >>> + int err;
> >>> +
> >>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >>> + return -ERANGE;
> >>> +
> >>> + err = switchdev_port_attr_set(br->dev, &attr);
> >>
> >> A thought - given that the ageing time is not a per-bridge-port attr, why
> >> are we using a "port based api"
> >> to pass the attribute down? May be I'm missing something here?
> >
> >I think Florian raised the same point earlier. Sigh, I think this
> >should be addressedv4 coming soon...thanks guys for keeping the
> >standard high.
>
> Scott, can you tell us how do you want to address this? I like the
> current implementation.
Scott, didn't you have a plan to add a struct device for the parent of
switchdev ports?
I think it would be good to introduce such device with an helper to
retrieve this upper parent, and move the switchdev ops to this guy.
So switchdev drivers may implement such API calls:
.obj_add(struct device *swdev, struct switchdev_obj *obj);
.port_obj_add(struct device *swdev, struct net_device *port,
struct switchdev_obj *obj);
Then switchdev code may have a parent API and the current port API may
look like this:
int switchdev_port_obj_add(struct net_device *dev,
struct switchdev_obj *obj)
{
struct device *swdev = switchdev_get_parent(dev);
int err = -EOPNOTSUPP;
if (swdev && swdev->switchdev_ops &&
swdev->switchdev_ops->port_obj_add)
err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
return err;
}
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
Sat, Oct 10, 2015 at 04:53:52AM CEST, sfel...@gmail.com wrote:
>On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala
>wrote:
>>
>>
>>> -Original Message-
>>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
>>> Sent: Friday, October 09, 2015 7:53 AM
>>> To: netdev@vger.kernel.org
>>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
>>> Premkumar Jonnala; step...@networkplumber.org;
>>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
>>> vivien.dide...@savoirfairelinux.com
>>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time
>>> down
>>> to switchdev
>>>
>>> From: Scott Feldman
>>>
>>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>>> support setting ageing_time (or setting bridge attrs in general).
>>>
>>> If push fails, don't update ageing_time in bridge and return err to user.
>>>
>>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>>> recalabrate when to run gc_timer next, based on new ageing_time.
>>>
>>> Signed-off-by: Scott Feldman
>>> Signed-off-by: Jiri Pirko
>
>
>
>>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>>> +{
>>> + struct switchdev_attr attr = {
>>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>> + .u.ageing_time = ageing_time,
>>> + };
>>> + unsigned long t = clock_t_to_jiffies(ageing_time);
>>> + int err;
>>> +
>>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>> + return -ERANGE;
>>> +
>>> + err = switchdev_port_attr_set(br->dev, &attr);
>>
>> A thought - given that the ageing time is not a per-bridge-port attr, why
>> are we using a "port based api"
>> to pass the attribute down? May be I'm missing something here?
>
>I think Florian raised the same point earlier. Sigh, I think this
>should be addressedv4 coming soon...thanks guys for keeping the
>standard high.
Scott, can you tell us how do you want to address this? I like the
current implementation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala wrote:
>
>
>> -Original Message-
>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
>> Sent: Friday, October 09, 2015 7:53 AM
>> To: netdev@vger.kernel.org
>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
>> Premkumar Jonnala; step...@networkplumber.org;
>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
>> vivien.dide...@savoirfairelinux.com
>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> to switchdev
>>
>> From: Scott Feldman
>>
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>>
>> If push fails, don't update ageing_time in bridge and return err to user.
>>
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>>
>> Signed-off-by: Scott Feldman
>> Signed-off-by: Jiri Pirko
>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> +{
>> + struct switchdev_attr attr = {
>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> + .u.ageing_time = ageing_time,
>> + };
>> + unsigned long t = clock_t_to_jiffies(ageing_time);
>> + int err;
>> +
>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> + return -ERANGE;
>> +
>> + err = switchdev_port_attr_set(br->dev, &attr);
>
> A thought - given that the ageing time is not a per-bridge-port attr, why are
> we using a "port based api"
> to pass the attribute down? May be I'm missing something here?
I think Florian raised the same point earlier. Sigh, I think this
should be addressedv4 coming soon...thanks guys for keeping the
standard high.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
Fri, Oct 09, 2015 at 06:38:10AM CEST, pjonn...@broadcom.com wrote:
>
>
>> -Original Message-
>> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
>> Sent: Friday, October 09, 2015 7:53 AM
>> To: netdev@vger.kernel.org
>> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
>> Premkumar Jonnala; step...@networkplumber.org;
>> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
>> vivien.dide...@savoirfairelinux.com
>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> to switchdev
>>
>> From: Scott Feldman
>>
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>>
>> If push fails, don't update ageing_time in bridge and return err to user.
>>
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>>
>> Signed-off-by: Scott Feldman
>> Signed-off-by: Jiri Pirko
>> ---
>> net/bridge/br_ioctl.c|3 +--
>> net/bridge/br_netlink.c |6 +++---
>> net/bridge/br_private.h |1 +
>> net/bridge/br_stp.c | 23 +++
>> net/bridge/br_sysfs_br.c |3 +--
>> 5 files changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index 8d423bc..263b4de 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
>> ifreq *rq, int cmd)
>> if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>> return -EPERM;
>>
>> -br->ageing_time = clock_t_to_jiffies(args[1]);
>> -return 0;
>> +return br_set_ageing_time(br, args[1]);
>>
>> case BRCTL_GET_PORT_INFO:
>> {
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index d78b442..544ab96 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -870,9 +870,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]);
>> -
>> -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 09d3ecb..ba0c67b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -882,6 +882,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 3a982c0..db6d243de 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -566,6 +566,29 @@ 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_ID_BRIDGE_AGEING_TIME,
>> +.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> +.u.ageing_time = ageing_time,
>> +};
>> +unsigned long t = clock_t_to_jiffies(ageing_time);
>> +int err;
>> +
>> +if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> +return -ERANGE;
>> +
>> +err = switchdev_port_attr_set(br->dev, &attr);
>
>A thought - given that the ageing time is not a per-bridge-port attr, why are
>we using a "port based api"
>to pass the attribute down? May be I'm missing something here?
I general, it can be. And in case of rocker, it is.
Other drivers will just use port handler to set the ageing time on the
appropriate bridge.
>
>-Prem
>
>
>> +if (err)
>> +return err;
>> +
>> +br->ageing_time = t;
>> +mod_timer(&br->gc_timer, jiffies);
>> +
>> +return 0;
>> +}
>> +
>> void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
>> {
>> br->bridge_forward_delay = t;
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 4c97fc5..04ef192 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -102,8 +102,7 @@ static ssize_t ageing_time_show(struct device *d,
>>
>> static int set_ageing_time(struct net_bridge *br, unsigned long val)
>> {
>> -br->ageing_time = clock_t_to_jiffies(val);
>> -return 0;
>> +return br_set_ageing_time(br, val);
>> }
>>
>> static ssize_t ageing_time_store(struct device *d,
>> --
>> 1.7.10
RE: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
> -Original Message-
> From: sfel...@gmail.com [mailto:sfel...@gmail.com]
> Sent: Friday, October 09, 2015 7:53 AM
> To: netdev@vger.kernel.org
> Cc: da...@davemloft.net; j...@resnulli.us; siva.mannem@gmail.com;
> Premkumar Jonnala; step...@networkplumber.org;
> ro...@cumulusnetworks.com; and...@lunn.ch; f.faine...@gmail.com;
> vivien.dide...@savoirfairelinux.com
> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
> to switchdev
>
> From: Scott Feldman
>
> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> support setting ageing_time (or setting bridge attrs in general).
>
> If push fails, don't update ageing_time in bridge and return err to user.
>
> If push succeeds, update ageing_time in bridge and run gc_timer now to
> recalabrate when to run gc_timer next, based on new ageing_time.
>
> Signed-off-by: Scott Feldman
> Signed-off-by: Jiri Pirko
> ---
> net/bridge/br_ioctl.c|3 +--
> net/bridge/br_netlink.c |6 +++---
> net/bridge/br_private.h |1 +
> net/bridge/br_stp.c | 23 +++
> net/bridge/br_sysfs_br.c |3 +--
> 5 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 8d423bc..263b4de 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
> ifreq *rq, int cmd)
> if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
> return -EPERM;
>
> - br->ageing_time = clock_t_to_jiffies(args[1]);
> - return 0;
> + return br_set_ageing_time(br, args[1]);
>
> case BRCTL_GET_PORT_INFO:
> {
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d78b442..544ab96 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -870,9 +870,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]);
> -
> - 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 09d3ecb..ba0c67b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -882,6 +882,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 3a982c0..db6d243de 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,29 @@ 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_ID_BRIDGE_AGEING_TIME,
> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> + .u.ageing_time = ageing_time,
> + };
> + unsigned long t = clock_t_to_jiffies(ageing_time);
> + int err;
> +
> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> + return -ERANGE;
> +
> + err = switchdev_port_attr_set(br->dev, &attr);
A thought - given that the ageing time is not a per-bridge-port attr, why are
we using a "port based api"
to pass the attribute down? May be I'm missing something here?
-Prem
> + if (err)
> + return err;
> +
> + br->ageing_time = t;
> + mod_timer(&br->gc_timer, jiffies);
> +
> + return 0;
> +}
> +
> void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
> {
> br->bridge_forward_delay = t;
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 4c97fc5..04ef192 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -102,8 +102,7 @@ static ssize_t ageing_time_show(struct device *d,
>
> static int set_ageing_time(struct net_bridge *br, unsigned long val)
> {
> - br->ageing_time = clock_t_to_jiffies(val);
> - return 0;
> + return br_set_ageing_time(br, val);
> }
>
> static ssize_t ageing_time_store(struct device *d,
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
Fri, Oct 09, 2015 at 04:23:19AM CEST, sfel...@gmail.com wrote: >From: Scott Feldman > >Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't >support setting ageing_time (or setting bridge attrs in general). > >If push fails, don't update ageing_time in bridge and return err to user. > >If push succeeds, update ageing_time in bridge and run gc_timer now to >recalabrate when to run gc_timer next, based on new ageing_time. > >Signed-off-by: Scott Feldman >Signed-off-by: Jiri Pirko Acked-by: Jiri Pirko > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
On Thu, Oct 8, 2015 at 7:40 PM, Florian Fainelli wrote:
> 2015-10-08 19:23 GMT-07:00 :
>> From: Scott Feldman
>>
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>>
>> If push fails, don't update ageing_time in bridge and return err to user.
>>
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>>
>> Signed-off-by: Scott Feldman
>> Signed-off-by: Jiri Pirko
>> ---
>> net/bridge/br_ioctl.c|3 +--
>> net/bridge/br_netlink.c |6 +++---
>> net/bridge/br_private.h |1 +
>> net/bridge/br_stp.c | 23 +++
>> net/bridge/br_sysfs_br.c |3 +--
>> 5 files changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index 8d423bc..263b4de 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
>> ifreq *rq, int cmd)
>> if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>> return -EPERM;
>>
>> - br->ageing_time = clock_t_to_jiffies(args[1]);
>> - return 0;
>> + return br_set_ageing_time(br, args[1]);
>>
>> case BRCTL_GET_PORT_INFO:
>> {
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index d78b442..544ab96 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -870,9 +870,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]);
>> -
>> - 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 09d3ecb..ba0c67b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -882,6 +882,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 3a982c0..db6d243de 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -566,6 +566,29 @@ 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_ID_BRIDGE_AGEING_TIME,
>> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> + .u.ageing_time = ageing_time,
>> + };
>> + unsigned long t = clock_t_to_jiffies(ageing_time);
>> + int err;
>> +
>> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> + return -ERANGE;
>> +
>> + err = switchdev_port_attr_set(br->dev, &attr);
>> + if (err)
>> + return err;
>> +
>> + br->ageing_time = t;
>> + mod_timer(&br->gc_timer, jiffies);
>
> If the switch driver/HW supports ageing, does it still make sense to
> have this software timer ticking?
Yes, because the bridge still needs to age out entries it has learned
(those not marked with added_by_external_learn), for example entries
learned on non-offloaded ports.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down to switchdev
2015-10-08 19:23 GMT-07:00 :
> From: Scott Feldman
>
> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> support setting ageing_time (or setting bridge attrs in general).
>
> If push fails, don't update ageing_time in bridge and return err to user.
>
> If push succeeds, update ageing_time in bridge and run gc_timer now to
> recalabrate when to run gc_timer next, based on new ageing_time.
>
> Signed-off-by: Scott Feldman
> Signed-off-by: Jiri Pirko
> ---
> net/bridge/br_ioctl.c|3 +--
> net/bridge/br_netlink.c |6 +++---
> net/bridge/br_private.h |1 +
> net/bridge/br_stp.c | 23 +++
> net/bridge/br_sysfs_br.c |3 +--
> 5 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 8d423bc..263b4de 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
> ifreq *rq, int cmd)
> if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
> return -EPERM;
>
> - br->ageing_time = clock_t_to_jiffies(args[1]);
> - return 0;
> + return br_set_ageing_time(br, args[1]);
>
> case BRCTL_GET_PORT_INFO:
> {
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d78b442..544ab96 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -870,9 +870,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]);
> -
> - 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 09d3ecb..ba0c67b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -882,6 +882,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 3a982c0..db6d243de 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,29 @@ 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_ID_BRIDGE_AGEING_TIME,
> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> + .u.ageing_time = ageing_time,
> + };
> + unsigned long t = clock_t_to_jiffies(ageing_time);
> + int err;
> +
> + if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> + return -ERANGE;
> +
> + err = switchdev_port_attr_set(br->dev, &attr);
> + if (err)
> + return err;
> +
> + br->ageing_time = t;
> + mod_timer(&br->gc_timer, jiffies);
If the switch driver/HW supports ageing, does it still make sense to
have this software timer ticking?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
