Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 02:55:01PM +0200, Vladimir Oltean wrote:
> On Wed, Feb 10, 2021 at 02:38:23PM +0200, Ido Schimmel wrote:
> > On Wed, Feb 10, 2021 at 02:29:36PM +0200, Vladimir Oltean wrote:
> > > On Wed, Feb 10, 2021 at 02:21:05PM +0200, Ido Schimmel wrote:
> > > > On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote:
> > > > > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> > > > > > On 10/02/2021 13:01, Vladimir Oltean wrote:
> > > > > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov
> > > > > > > wrote:
> > > > > > >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > > > > > >>> Hi Nikolay,
> > > > > > >>>
> > > > > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov
> > > > > > >>> wrote:
> > > > > > Hi Vladimir,
> > > > > > Let's take a step back for a moment and discuss the bridge
> > > > > > unlock/lock sequences
> > > > > > that come with this set. I'd really like to avoid those as
> > > > > > they're a recipe
> > > > > > for future problems. The only good way to achieve that
> > > > > > currently is to keep
> > > > > > the PRE_FLAGS call and do that in unsleepable context but move
> > > > > > the FLAGS call
> > > > > > after the flags have been changed (if they have changed
> > > > > > obviously). That would
> > > > > > make the code read much easier since we'll have all our
> > > > > > lock/unlock sequences
> > > > > > in the same code blocks and won't play games to get sleepable
> > > > > > context.
> > > > > > Please let's think and work in that direction, rather than
> > > > > > having:
> > > > > > + spin_lock_bh(&p->br->lock);
> > > > > > + if (err) {
> > > > > > + netdev_err(p->dev, "%s\n", extack._msg);
> > > > > > + return err;
> > > > > > }
> > > > > > +
> > > > > >
> > > > > > which immediately looks like a bug even though after some code
> > > > > > checking we can
> > > > > > verify it's ok. WDYT?
> > > > > >
> > > > > > I plan to get rid of most of the br->lock since it's been
> > > > > > abused for a very long
> > > > > > time because it's essentially STP lock, but people have
> > > > > > started using it for other
> > > > > > things and I plan to fix that when I get more time.
> > > > > > >>>
> > > > > > >>> This won't make the sysfs codepath any nicer, will it?
> > > > > > >>>
> > > > > > >>
> > > > > > >> Currently we'll have to live with a hack that checks if the
> > > > > > >> flags have changed. I agree
> > > > > > >> it won't be pretty, but we won't have to unlock and lock again
> > > > > > >> in the middle of the
> > > > > > >> called function and we'll have all our locking in the same
> > > > > > >> place, easier to verify and
> > > > > > >> later easier to remove. Once I get rid of most of the br->lock
> > > > > > >> usage we can revisit
> > > > > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to
> > > > > > >> change the flags, then
> > > > > > >> send the switchdev notification outside of the lock and revert
> > > > > > >> the flags if it doesn't
> > > > > > >> go through which doesn't sound much better.
> > > > > > >> I'm open to any other suggestions, but definitely would like to
> > > > > > >> avoid playing locking games.
> > > > > > >> Even if it means casing out flag setting from all other store_
> > > > > > >> functions for sysfs.
> > > > > > >
> > > > > > > By casing out flag settings you mean something like this?
> > > > > > >
> > > > > > >
> > > > > > > #define BRPORT_ATTR(_name, _mode, _show, _store) \
> > > > > > > const struct brport_attribute brport_attr_##_name = {
> > > > > > > \
> > > > > > > .attr = {.name = __stringify(_name),\
> > > > > > >.mode = _mode }, \
> > > > > > > .show = _show,\
> > > > > > > .store_unlocked = _store, \
> > > > > > > };
> > > > > > >
> > > > > > > #define BRPORT_ATTR_FLAG(_name, _mask)
> > > > > > > \
> > > > > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf)
> > > > > > > \
> > > > > > > { \
> > > > > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > > > > > > } \
> > > > > > > static int store_##_name(struct net_bridge_port *p, unsigned long
> > > > > > > v) \
> > > > > > > { \
> > > > > > > return store_flag(p, v, _mask); \
> > > > > > > } \
> > > > > > > static BRPORT_ATTR(_name, 0644,
> > > > > > > \
>
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 02:38:23PM +0200, Ido Schimmel wrote:
> On Wed, Feb 10, 2021 at 02:29:36PM +0200, Vladimir Oltean wrote:
> > On Wed, Feb 10, 2021 at 02:21:05PM +0200, Ido Schimmel wrote:
> > > On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote:
> > > > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> > > > > On 10/02/2021 13:01, Vladimir Oltean wrote:
> > > > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> > > > > >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > > > > >>> Hi Nikolay,
> > > > > >>>
> > > > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov
> > > > > >>> wrote:
> > > > > Hi Vladimir,
> > > > > Let's take a step back for a moment and discuss the bridge
> > > > > unlock/lock sequences
> > > > > that come with this set. I'd really like to avoid those as
> > > > > they're a recipe
> > > > > for future problems. The only good way to achieve that currently
> > > > > is to keep
> > > > > the PRE_FLAGS call and do that in unsleepable context but move
> > > > > the FLAGS call
> > > > > after the flags have been changed (if they have changed
> > > > > obviously). That would
> > > > > make the code read much easier since we'll have all our
> > > > > lock/unlock sequences
> > > > > in the same code blocks and won't play games to get sleepable
> > > > > context.
> > > > > Please let's think and work in that direction, rather than
> > > > > having:
> > > > > +spin_lock_bh(&p->br->lock);
> > > > > +if (err) {
> > > > > +netdev_err(p->dev, "%s\n", extack._msg);
> > > > > +return err;
> > > > > }
> > > > > +
> > > > >
> > > > > which immediately looks like a bug even though after some code
> > > > > checking we can
> > > > > verify it's ok. WDYT?
> > > > >
> > > > > I plan to get rid of most of the br->lock since it's been abused
> > > > > for a very long
> > > > > time because it's essentially STP lock, but people have started
> > > > > using it for other
> > > > > things and I plan to fix that when I get more time.
> > > > > >>>
> > > > > >>> This won't make the sysfs codepath any nicer, will it?
> > > > > >>>
> > > > > >>
> > > > > >> Currently we'll have to live with a hack that checks if the flags
> > > > > >> have changed. I agree
> > > > > >> it won't be pretty, but we won't have to unlock and lock again in
> > > > > >> the middle of the
> > > > > >> called function and we'll have all our locking in the same place,
> > > > > >> easier to verify and
> > > > > >> later easier to remove. Once I get rid of most of the br->lock
> > > > > >> usage we can revisit
> > > > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to
> > > > > >> change the flags, then
> > > > > >> send the switchdev notification outside of the lock and revert the
> > > > > >> flags if it doesn't
> > > > > >> go through which doesn't sound much better.
> > > > > >> I'm open to any other suggestions, but definitely would like to
> > > > > >> avoid playing locking games.
> > > > > >> Even if it means casing out flag setting from all other store_
> > > > > >> functions for sysfs.
> > > > > >
> > > > > > By casing out flag settings you mean something like this?
> > > > > >
> > > > > >
> > > > > > #define BRPORT_ATTR(_name, _mode, _show, _store)\
> > > > > > const struct brport_attribute brport_attr_##_name = {
> > > > > > \
> > > > > > .attr = {.name = __stringify(_name),\
> > > > > > .mode = _mode }, \
> > > > > > .show = _show,\
> > > > > > .store_unlocked = _store, \
> > > > > > };
> > > > > >
> > > > > > #define BRPORT_ATTR_FLAG(_name, _mask)
> > > > > > \
> > > > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> > > > > > { \
> > > > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > > > > > } \
> > > > > > static int store_##_name(struct net_bridge_port *p, unsigned long
> > > > > > v) \
> > > > > > { \
> > > > > > return store_flag(p, v, _mask); \
> > > > > > } \
> > > > > > static BRPORT_ATTR(_name, 0644,
> > > > > > \
> > > > > >show_##_name, store_##_name)
> > > > > >
> > > > > > static ssize_t brport_store(struct kobject *kobj,
> > > > > > struct attribute *attr,
> > > > > > const char *buf,
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 02:29:36PM +0200, Vladimir Oltean wrote:
> On Wed, Feb 10, 2021 at 02:21:05PM +0200, Ido Schimmel wrote:
> > On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote:
> > > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> > > > On 10/02/2021 13:01, Vladimir Oltean wrote:
> > > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> > > > >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > > > >>> Hi Nikolay,
> > > > >>>
> > > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> > > > Hi Vladimir,
> > > > Let's take a step back for a moment and discuss the bridge
> > > > unlock/lock sequences
> > > > that come with this set. I'd really like to avoid those as they're
> > > > a recipe
> > > > for future problems. The only good way to achieve that currently
> > > > is to keep
> > > > the PRE_FLAGS call and do that in unsleepable context but move the
> > > > FLAGS call
> > > > after the flags have been changed (if they have changed
> > > > obviously). That would
> > > > make the code read much easier since we'll have all our
> > > > lock/unlock sequences
> > > > in the same code blocks and won't play games to get sleepable
> > > > context.
> > > > Please let's think and work in that direction, rather than having:
> > > > + spin_lock_bh(&p->br->lock);
> > > > + if (err) {
> > > > + netdev_err(p->dev, "%s\n", extack._msg);
> > > > + return err;
> > > > }
> > > > +
> > > >
> > > > which immediately looks like a bug even though after some code
> > > > checking we can
> > > > verify it's ok. WDYT?
> > > >
> > > > I plan to get rid of most of the br->lock since it's been abused
> > > > for a very long
> > > > time because it's essentially STP lock, but people have started
> > > > using it for other
> > > > things and I plan to fix that when I get more time.
> > > > >>>
> > > > >>> This won't make the sysfs codepath any nicer, will it?
> > > > >>>
> > > > >>
> > > > >> Currently we'll have to live with a hack that checks if the flags
> > > > >> have changed. I agree
> > > > >> it won't be pretty, but we won't have to unlock and lock again in
> > > > >> the middle of the
> > > > >> called function and we'll have all our locking in the same place,
> > > > >> easier to verify and
> > > > >> later easier to remove. Once I get rid of most of the br->lock usage
> > > > >> we can revisit
> > > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to
> > > > >> change the flags, then
> > > > >> send the switchdev notification outside of the lock and revert the
> > > > >> flags if it doesn't
> > > > >> go through which doesn't sound much better.
> > > > >> I'm open to any other suggestions, but definitely would like to
> > > > >> avoid playing locking games.
> > > > >> Even if it means casing out flag setting from all other store_
> > > > >> functions for sysfs.
> > > > >
> > > > > By casing out flag settings you mean something like this?
> > > > >
> > > > >
> > > > > #define BRPORT_ATTR(_name, _mode, _show, _store) \
> > > > > const struct brport_attribute brport_attr_##_name = {
> > > > > \
> > > > > .attr = {.name = __stringify(_name),\
> > > > >.mode = _mode }, \
> > > > > .show = _show,\
> > > > > .store_unlocked = _store, \
> > > > > };
> > > > >
> > > > > #define BRPORT_ATTR_FLAG(_name, _mask)
> > > > > \
> > > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> > > > > { \
> > > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > > > > } \
> > > > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \
> > > > > { \
> > > > > return store_flag(p, v, _mask); \
> > > > > } \
> > > > > static BRPORT_ATTR(_name, 0644,
> > > > > \
> > > > > show_##_name, store_##_name)
> > > > >
> > > > > static ssize_t brport_store(struct kobject *kobj,
> > > > > struct attribute *attr,
> > > > > const char *buf, size_t count)
> > > > > {
> > > > > ...
> > > > >
> > > > > } else if (brport_attr->store_unlocked) {
> > > > > val = simple_strtoul(buf, &endp, 0);
> > > > > if (endp == buf)
> > > > > goto out_unlock;
> > > > >
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 02:21:05PM +0200, Ido Schimmel wrote:
> On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote:
> > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> > > On 10/02/2021 13:01, Vladimir Oltean wrote:
> > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> > > >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > > >>> Hi Nikolay,
> > > >>>
> > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> > > Hi Vladimir,
> > > Let's take a step back for a moment and discuss the bridge
> > > unlock/lock sequences
> > > that come with this set. I'd really like to avoid those as they're a
> > > recipe
> > > for future problems. The only good way to achieve that currently is
> > > to keep
> > > the PRE_FLAGS call and do that in unsleepable context but move the
> > > FLAGS call
> > > after the flags have been changed (if they have changed obviously).
> > > That would
> > > make the code read much easier since we'll have all our lock/unlock
> > > sequences
> > > in the same code blocks and won't play games to get sleepable
> > > context.
> > > Please let's think and work in that direction, rather than having:
> > > +spin_lock_bh(&p->br->lock);
> > > +if (err) {
> > > +netdev_err(p->dev, "%s\n", extack._msg);
> > > +return err;
> > > }
> > > +
> > >
> > > which immediately looks like a bug even though after some code
> > > checking we can
> > > verify it's ok. WDYT?
> > >
> > > I plan to get rid of most of the br->lock since it's been abused for
> > > a very long
> > > time because it's essentially STP lock, but people have started
> > > using it for other
> > > things and I plan to fix that when I get more time.
> > > >>>
> > > >>> This won't make the sysfs codepath any nicer, will it?
> > > >>>
> > > >>
> > > >> Currently we'll have to live with a hack that checks if the flags have
> > > >> changed. I agree
> > > >> it won't be pretty, but we won't have to unlock and lock again in the
> > > >> middle of the
> > > >> called function and we'll have all our locking in the same place,
> > > >> easier to verify and
> > > >> later easier to remove. Once I get rid of most of the br->lock usage
> > > >> we can revisit
> > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to change
> > > >> the flags, then
> > > >> send the switchdev notification outside of the lock and revert the
> > > >> flags if it doesn't
> > > >> go through which doesn't sound much better.
> > > >> I'm open to any other suggestions, but definitely would like to avoid
> > > >> playing locking games.
> > > >> Even if it means casing out flag setting from all other store_
> > > >> functions for sysfs.
> > > >
> > > > By casing out flag settings you mean something like this?
> > > >
> > > >
> > > > #define BRPORT_ATTR(_name, _mode, _show, _store)\
> > > > const struct brport_attribute brport_attr_##_name = { \
> > > > .attr = {.name = __stringify(_name),\
> > > > .mode = _mode }, \
> > > > .show = _show,\
> > > > .store_unlocked = _store, \
> > > > };
> > > >
> > > > #define BRPORT_ATTR_FLAG(_name, _mask) \
> > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> > > > { \
> > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > > > } \
> > > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \
> > > > { \
> > > > return store_flag(p, v, _mask); \
> > > > } \
> > > > static BRPORT_ATTR(_name, 0644, \
> > > >show_##_name, store_##_name)
> > > >
> > > > static ssize_t brport_store(struct kobject *kobj,
> > > > struct attribute *attr,
> > > > const char *buf, size_t count)
> > > > {
> > > > ...
> > > >
> > > > } else if (brport_attr->store_unlocked) {
> > > > val = simple_strtoul(buf, &endp, 0);
> > > > if (endp == buf)
> > > > goto out_unlock;
> > > > ret = brport_attr->store_unlocked(p, val);
> > > > }
> > > >
> > >
> > > Yes, this can work but will need a bit more changes because of
> > > br_port_flags_change().
> > > Then the netlink side can be modeled in a similar way.
> >
> > What I just do
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote:
> On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> > On 10/02/2021 13:01, Vladimir Oltean wrote:
> > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> > >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > >>> Hi Nikolay,
> > >>>
> > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> > Hi Vladimir,
> > Let's take a step back for a moment and discuss the bridge unlock/lock
> > sequences
> > that come with this set. I'd really like to avoid those as they're a
> > recipe
> > for future problems. The only good way to achieve that currently is to
> > keep
> > the PRE_FLAGS call and do that in unsleepable context but move the
> > FLAGS call
> > after the flags have been changed (if they have changed obviously).
> > That would
> > make the code read much easier since we'll have all our lock/unlock
> > sequences
> > in the same code blocks and won't play games to get sleepable context.
> > Please let's think and work in that direction, rather than having:
> > + spin_lock_bh(&p->br->lock);
> > + if (err) {
> > + netdev_err(p->dev, "%s\n", extack._msg);
> > + return err;
> > }
> > +
> >
> > which immediately looks like a bug even though after some code
> > checking we can
> > verify it's ok. WDYT?
> >
> > I plan to get rid of most of the br->lock since it's been abused for a
> > very long
> > time because it's essentially STP lock, but people have started using
> > it for other
> > things and I plan to fix that when I get more time.
> > >>>
> > >>> This won't make the sysfs codepath any nicer, will it?
> > >>>
> > >>
> > >> Currently we'll have to live with a hack that checks if the flags have
> > >> changed. I agree
> > >> it won't be pretty, but we won't have to unlock and lock again in the
> > >> middle of the
> > >> called function and we'll have all our locking in the same place, easier
> > >> to verify and
> > >> later easier to remove. Once I get rid of most of the br->lock usage we
> > >> can revisit
> > >> the drop of PRE_FLAGS if it's a problem. The alternative is to change
> > >> the flags, then
> > >> send the switchdev notification outside of the lock and revert the flags
> > >> if it doesn't
> > >> go through which doesn't sound much better.
> > >> I'm open to any other suggestions, but definitely would like to avoid
> > >> playing locking games.
> > >> Even if it means casing out flag setting from all other store_ functions
> > >> for sysfs.
> > >
> > > By casing out flag settings you mean something like this?
> > >
> > >
> > > #define BRPORT_ATTR(_name, _mode, _show, _store) \
> > > const struct brport_attribute brport_attr_##_name = { \
> > > .attr = {.name = __stringify(_name),\
> > >.mode = _mode }, \
> > > .show = _show,\
> > > .store_unlocked = _store, \
> > > };
> > >
> > > #define BRPORT_ATTR_FLAG(_name, _mask)\
> > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> > > { \
> > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > > } \
> > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \
> > > { \
> > > return store_flag(p, v, _mask); \
> > > } \
> > > static BRPORT_ATTR(_name, 0644, \
> > > show_##_name, store_##_name)
> > >
> > > static ssize_t brport_store(struct kobject *kobj,
> > > struct attribute *attr,
> > > const char *buf, size_t count)
> > > {
> > > ...
> > >
> > > } else if (brport_attr->store_unlocked) {
> > > val = simple_strtoul(buf, &endp, 0);
> > > if (endp == buf)
> > > goto out_unlock;
> > > ret = brport_attr->store_unlocked(p, val);
> > > }
> > >
> >
> > Yes, this can work but will need a bit more changes because of
> > br_port_flags_change().
> > Then the netlink side can be modeled in a similar way.
>
> What I just don't understand is how others can get away with doing
> sleepable work in atomic context but I can't make the notifier blocking
> by dropping a spinlock which isn't needed there, because it looks ugly :D
Can you please point to the bug? I'm not following
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On 10/02/2021 14:01, Vladimir Oltean wrote:
> On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
>> On 10/02/2021 13:01, Vladimir Oltean wrote:
>>> On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
On 10/02/2021 12:45, Vladimir Oltean wrote:
> Hi Nikolay,
>
> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
>> Hi Vladimir,
>> Let's take a step back for a moment and discuss the bridge unlock/lock
>> sequences
>> that come with this set. I'd really like to avoid those as they're a
>> recipe
>> for future problems. The only good way to achieve that currently is to
>> keep
>> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS
>> call
>> after the flags have been changed (if they have changed obviously). That
>> would
>> make the code read much easier since we'll have all our lock/unlock
>> sequences
>> in the same code blocks and won't play games to get sleepable context.
>> Please let's think and work in that direction, rather than having:
>> +spin_lock_bh(&p->br->lock);
>> +if (err) {
>> +netdev_err(p->dev, "%s\n", extack._msg);
>> +return err;
>> }
>> +
>>
>> which immediately looks like a bug even though after some code checking
>> we can
>> verify it's ok. WDYT?
>>
>> I plan to get rid of most of the br->lock since it's been abused for a
>> very long
>> time because it's essentially STP lock, but people have started using it
>> for other
>> things and I plan to fix that when I get more time.
>
> This won't make the sysfs codepath any nicer, will it?
>
Currently we'll have to live with a hack that checks if the flags have
changed. I agree
it won't be pretty, but we won't have to unlock and lock again in the
middle of the
called function and we'll have all our locking in the same place, easier
to verify and
later easier to remove. Once I get rid of most of the br->lock usage we
can revisit
the drop of PRE_FLAGS if it's a problem. The alternative is to change the
flags, then
send the switchdev notification outside of the lock and revert the flags
if it doesn't
go through which doesn't sound much better.
I'm open to any other suggestions, but definitely would like to avoid
playing locking games.
Even if it means casing out flag setting from all other store_ functions
for sysfs.
>>>
>>> By casing out flag settings you mean something like this?
>>>
>>>
>>> #define BRPORT_ATTR(_name, _mode, _show, _store)\
>>> const struct brport_attribute brport_attr_##_name = { \
>>> .attr = {.name = __stringify(_name),\
>>> .mode = _mode }, \
>>> .show = _show,\
>>> .store_unlocked = _store, \
>>> };
>>>
>>> #define BRPORT_ATTR_FLAG(_name, _mask) \
>>> static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
>>> { \
>>> return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
>>> } \
>>> static int store_##_name(struct net_bridge_port *p, unsigned long v) \
>>> { \
>>> return store_flag(p, v, _mask); \
>>> } \
>>> static BRPORT_ATTR(_name, 0644, \
>>>show_##_name, store_##_name)
>>>
>>> static ssize_t brport_store(struct kobject *kobj,
>>> struct attribute *attr,
>>> const char *buf, size_t count)
>>> {
>>> ...
>>>
>>> } else if (brport_attr->store_unlocked) {
>>> val = simple_strtoul(buf, &endp, 0);
>>> if (endp == buf)
>>> goto out_unlock;
>>> ret = brport_attr->store_unlocked(p, val);
>>> }
>>>
>>
>> Yes, this can work but will need a bit more changes because of
>> br_port_flags_change().
>> Then the netlink side can be modeled in a similar way.
>
> What I just don't understand is how others can get away with doing
> sleepable work in atomic context but I can't make the notifier blocking
> by dropping a spinlock which isn't needed there, because it looks ugly :D
>
That's a bug that's gone unnoticed, surely not an argument to make error-prone
changes.
It's not because of ugliness, rather for easier reasoning when people want to
work with
that code, easier to maintain and later easier to verify when the lock gets
removed.
We'll reduce the chance for new bugs by having code that can be understood
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote:
> On 10/02/2021 13:01, Vladimir Oltean wrote:
> > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> >> On 10/02/2021 12:45, Vladimir Oltean wrote:
> >>> Hi Nikolay,
> >>>
> >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> Hi Vladimir,
> Let's take a step back for a moment and discuss the bridge unlock/lock
> sequences
> that come with this set. I'd really like to avoid those as they're a
> recipe
> for future problems. The only good way to achieve that currently is to
> keep
> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS
> call
> after the flags have been changed (if they have changed obviously). That
> would
> make the code read much easier since we'll have all our lock/unlock
> sequences
> in the same code blocks and won't play games to get sleepable context.
> Please let's think and work in that direction, rather than having:
> +spin_lock_bh(&p->br->lock);
> +if (err) {
> +netdev_err(p->dev, "%s\n", extack._msg);
> +return err;
> }
> +
>
> which immediately looks like a bug even though after some code checking
> we can
> verify it's ok. WDYT?
>
> I plan to get rid of most of the br->lock since it's been abused for a
> very long
> time because it's essentially STP lock, but people have started using it
> for other
> things and I plan to fix that when I get more time.
> >>>
> >>> This won't make the sysfs codepath any nicer, will it?
> >>>
> >>
> >> Currently we'll have to live with a hack that checks if the flags have
> >> changed. I agree
> >> it won't be pretty, but we won't have to unlock and lock again in the
> >> middle of the
> >> called function and we'll have all our locking in the same place, easier
> >> to verify and
> >> later easier to remove. Once I get rid of most of the br->lock usage we
> >> can revisit
> >> the drop of PRE_FLAGS if it's a problem. The alternative is to change the
> >> flags, then
> >> send the switchdev notification outside of the lock and revert the flags
> >> if it doesn't
> >> go through which doesn't sound much better.
> >> I'm open to any other suggestions, but definitely would like to avoid
> >> playing locking games.
> >> Even if it means casing out flag setting from all other store_ functions
> >> for sysfs.
> >
> > By casing out flag settings you mean something like this?
> >
> >
> > #define BRPORT_ATTR(_name, _mode, _show, _store)\
> > const struct brport_attribute brport_attr_##_name = { \
> > .attr = {.name = __stringify(_name),\
> > .mode = _mode }, \
> > .show = _show,\
> > .store_unlocked = _store, \
> > };
> >
> > #define BRPORT_ATTR_FLAG(_name, _mask) \
> > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> > { \
> > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> > } \
> > static int store_##_name(struct net_bridge_port *p, unsigned long v) \
> > { \
> > return store_flag(p, v, _mask); \
> > } \
> > static BRPORT_ATTR(_name, 0644, \
> >show_##_name, store_##_name)
> >
> > static ssize_t brport_store(struct kobject *kobj,
> > struct attribute *attr,
> > const char *buf, size_t count)
> > {
> > ...
> >
> > } else if (brport_attr->store_unlocked) {
> > val = simple_strtoul(buf, &endp, 0);
> > if (endp == buf)
> > goto out_unlock;
> > ret = brport_attr->store_unlocked(p, val);
> > }
> >
>
> Yes, this can work but will need a bit more changes because of
> br_port_flags_change().
> Then the netlink side can be modeled in a similar way.
What I just don't understand is how others can get away with doing
sleepable work in atomic context but I can't make the notifier blocking
by dropping a spinlock which isn't needed there, because it looks ugly :D
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On 10/02/2021 13:01, Vladimir Oltean wrote:
> On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
>> On 10/02/2021 12:45, Vladimir Oltean wrote:
>>> Hi Nikolay,
>>>
>>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
Hi Vladimir,
Let's take a step back for a moment and discuss the bridge unlock/lock
sequences
that come with this set. I'd really like to avoid those as they're a recipe
for future problems. The only good way to achieve that currently is to keep
the PRE_FLAGS call and do that in unsleepable context but move the FLAGS
call
after the flags have been changed (if they have changed obviously). That
would
make the code read much easier since we'll have all our lock/unlock
sequences
in the same code blocks and won't play games to get sleepable context.
Please let's think and work in that direction, rather than having:
+ spin_lock_bh(&p->br->lock);
+ if (err) {
+ netdev_err(p->dev, "%s\n", extack._msg);
+ return err;
}
+
which immediately looks like a bug even though after some code checking we
can
verify it's ok. WDYT?
I plan to get rid of most of the br->lock since it's been abused for a
very long
time because it's essentially STP lock, but people have started using it
for other
things and I plan to fix that when I get more time.
>>>
>>> This won't make the sysfs codepath any nicer, will it?
>>>
>>
>> Currently we'll have to live with a hack that checks if the flags have
>> changed. I agree
>> it won't be pretty, but we won't have to unlock and lock again in the middle
>> of the
>> called function and we'll have all our locking in the same place, easier to
>> verify and
>> later easier to remove. Once I get rid of most of the br->lock usage we can
>> revisit
>> the drop of PRE_FLAGS if it's a problem. The alternative is to change the
>> flags, then
>> send the switchdev notification outside of the lock and revert the flags if
>> it doesn't
>> go through which doesn't sound much better.
>> I'm open to any other suggestions, but definitely would like to avoid
>> playing locking games.
>> Even if it means casing out flag setting from all other store_ functions for
>> sysfs.
>
> By casing out flag settings you mean something like this?
>
>
> #define BRPORT_ATTR(_name, _mode, _show, _store) \
> const struct brport_attribute brport_attr_##_name = { \
> .attr = {.name = __stringify(_name),\
>.mode = _mode }, \
> .show = _show,\
> .store_unlocked = _store, \
> };
>
> #define BRPORT_ATTR_FLAG(_name, _mask)\
> static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
> { \
> return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
> } \
> static int store_##_name(struct net_bridge_port *p, unsigned long v) \
> { \
> return store_flag(p, v, _mask); \
> } \
> static BRPORT_ATTR(_name, 0644, \
> show_##_name, store_##_name)
>
> static ssize_t brport_store(struct kobject *kobj,
> struct attribute *attr,
> const char *buf, size_t count)
> {
> ...
>
> } else if (brport_attr->store_unlocked) {
> val = simple_strtoul(buf, &endp, 0);
> if (endp == buf)
> goto out_unlock;
> ret = brport_attr->store_unlocked(p, val);
> }
>
Yes, this can work but will need a bit more changes because of
br_port_flags_change().
Then the netlink side can be modeled in a similar way.
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote:
> On 10/02/2021 12:45, Vladimir Oltean wrote:
> > Hi Nikolay,
> >
> > On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> >> Hi Vladimir,
> >> Let's take a step back for a moment and discuss the bridge unlock/lock
> >> sequences
> >> that come with this set. I'd really like to avoid those as they're a recipe
> >> for future problems. The only good way to achieve that currently is to keep
> >> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS
> >> call
> >> after the flags have been changed (if they have changed obviously). That
> >> would
> >> make the code read much easier since we'll have all our lock/unlock
> >> sequences
> >> in the same code blocks and won't play games to get sleepable context.
> >> Please let's think and work in that direction, rather than having:
> >> + spin_lock_bh(&p->br->lock);
> >> + if (err) {
> >> + netdev_err(p->dev, "%s\n", extack._msg);
> >> + return err;
> >>}
> >> +
> >>
> >> which immediately looks like a bug even though after some code checking we
> >> can
> >> verify it's ok. WDYT?
> >>
> >> I plan to get rid of most of the br->lock since it's been abused for a
> >> very long
> >> time because it's essentially STP lock, but people have started using it
> >> for other
> >> things and I plan to fix that when I get more time.
> >
> > This won't make the sysfs codepath any nicer, will it?
> >
>
> Currently we'll have to live with a hack that checks if the flags have
> changed. I agree
> it won't be pretty, but we won't have to unlock and lock again in the middle
> of the
> called function and we'll have all our locking in the same place, easier to
> verify and
> later easier to remove. Once I get rid of most of the br->lock usage we can
> revisit
> the drop of PRE_FLAGS if it's a problem. The alternative is to change the
> flags, then
> send the switchdev notification outside of the lock and revert the flags if
> it doesn't
> go through which doesn't sound much better.
> I'm open to any other suggestions, but definitely would like to avoid playing
> locking games.
> Even if it means casing out flag setting from all other store_ functions for
> sysfs.
By casing out flag settings you mean something like this?
#define BRPORT_ATTR(_name, _mode, _show, _store)\
const struct brport_attribute brport_attr_##_name = { \
.attr = {.name = __stringify(_name),\
.mode = _mode }, \
.show = _show,\
.store_unlocked = _store, \
};
#define BRPORT_ATTR_FLAG(_name, _mask) \
static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \
{ \
return sprintf(buf, "%d\n", !!(p->flags & _mask)); \
} \
static int store_##_name(struct net_bridge_port *p, unsigned long v) \
{ \
return store_flag(p, v, _mask); \
} \
static BRPORT_ATTR(_name, 0644, \
show_##_name, store_##_name)
static ssize_t brport_store(struct kobject *kobj,
struct attribute *attr,
const char *buf, size_t count)
{
...
} else if (brport_attr->store_unlocked) {
val = simple_strtoul(buf, &endp, 0);
if (endp == buf)
goto out_unlock;
ret = brport_attr->store_unlocked(p, val);
}
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On 10/02/2021 12:45, Vladimir Oltean wrote:
> Hi Nikolay,
>
> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
>> Hi Vladimir,
>> Let's take a step back for a moment and discuss the bridge unlock/lock
>> sequences
>> that come with this set. I'd really like to avoid those as they're a recipe
>> for future problems. The only good way to achieve that currently is to keep
>> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call
>> after the flags have been changed (if they have changed obviously). That
>> would
>> make the code read much easier since we'll have all our lock/unlock sequences
>> in the same code blocks and won't play games to get sleepable context.
>> Please let's think and work in that direction, rather than having:
>> +spin_lock_bh(&p->br->lock);
>> +if (err) {
>> +netdev_err(p->dev, "%s\n", extack._msg);
>> +return err;
>> }
>> +
>>
>> which immediately looks like a bug even though after some code checking we
>> can
>> verify it's ok. WDYT?
>>
>> I plan to get rid of most of the br->lock since it's been abused for a very
>> long
>> time because it's essentially STP lock, but people have started using it for
>> other
>> things and I plan to fix that when I get more time.
>
> This won't make the sysfs codepath any nicer, will it?
>
Currently we'll have to live with a hack that checks if the flags have changed.
I agree
it won't be pretty, but we won't have to unlock and lock again in the middle of
the
called function and we'll have all our locking in the same place, easier to
verify and
later easier to remove. Once I get rid of most of the br->lock usage we can
revisit
the drop of PRE_FLAGS if it's a problem. The alternative is to change the
flags, then
send the switchdev notification outside of the lock and revert the flags if it
doesn't
go through which doesn't sound much better.
I'm open to any other suggestions, but definitely would like to avoid playing
locking games.
Even if it means casing out flag setting from all other store_ functions for
sysfs.
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
Hi Nikolay,
On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote:
> Hi Vladimir,
> Let's take a step back for a moment and discuss the bridge unlock/lock
> sequences
> that come with this set. I'd really like to avoid those as they're a recipe
> for future problems. The only good way to achieve that currently is to keep
> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call
> after the flags have been changed (if they have changed obviously). That would
> make the code read much easier since we'll have all our lock/unlock sequences
> in the same code blocks and won't play games to get sleepable context.
> Please let's think and work in that direction, rather than having:
> + spin_lock_bh(&p->br->lock);
> + if (err) {
> + netdev_err(p->dev, "%s\n", extack._msg);
> + return err;
> }
> +
>
> which immediately looks like a bug even though after some code checking we can
> verify it's ok. WDYT?
>
> I plan to get rid of most of the br->lock since it's been abused for a very
> long
> time because it's essentially STP lock, but people have started using it for
> other
> things and I plan to fix that when I get more time.
This won't make the sysfs codepath any nicer, will it?
Re: [PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
On 10/02/2021 11:14, Vladimir Oltean wrote:
> From: Vladimir Oltean
>
> The initial goal of this series was to have better support for
> standalone ports mode and multiple bridges on the DSA drivers like
> ocelot/felix and sja1105. Proper support for standalone mode requires
> disabling address learning, which in turn requires interaction with the
> switchdev notifier, which is actually where most of the patches are.
>
> I also noticed that most of the drivers are actually talking either to
> firmware or SPI/MDIO connected devices from the brport flags switchdev
> attribute handler, so it makes sense to actually make it sleepable
> instead of atomic.
>
Hi Vladimir,
Let's take a step back for a moment and discuss the bridge unlock/lock sequences
that come with this set. I'd really like to avoid those as they're a recipe
for future problems. The only good way to achieve that currently is to keep
the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call
after the flags have been changed (if they have changed obviously). That would
make the code read much easier since we'll have all our lock/unlock sequences
in the same code blocks and won't play games to get sleepable context.
Please let's think and work in that direction, rather than having:
+ spin_lock_bh(&p->br->lock);
+ if (err) {
+ netdev_err(p->dev, "%s\n", extack._msg);
+ return err;
}
+
which immediately looks like a bug even though after some code checking we can
verify it's ok. WDYT?
I plan to get rid of most of the br->lock since it's been abused for a very long
time because it's essentially STP lock, but people have started using it for
other
things and I plan to fix that when I get more time.
Thanks,
Nik
[PATCH v3 net-next 00/11] Cleanup in brport flags switchdev offload for DSA
From: Vladimir Oltean The initial goal of this series was to have better support for standalone ports mode and multiple bridges on the DSA drivers like ocelot/felix and sja1105. Proper support for standalone mode requires disabling address learning, which in turn requires interaction with the switchdev notifier, which is actually where most of the patches are. I also noticed that most of the drivers are actually talking either to firmware or SPI/MDIO connected devices from the brport flags switchdev attribute handler, so it makes sense to actually make it sleepable instead of atomic. Vladimir Oltean (11): net: switchdev: propagate extack to port attributes net: bridge: offload all port flags at once in br_setport net: bridge: don't print in br_switchdev_set_port_flag net: dsa: configure proper brport flags when ports leave the bridge net: squash switchdev attributes PRE_BRIDGE_FLAGS and BRIDGE_FLAGS net: dsa: kill .port_egress_floods overengineering net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS net: bridge: put SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS on the blocking call chain net: mscc: ocelot: use separate flooding PGID for broadcast net: mscc: ocelot: offload bridge port flags to device net: dsa: sja1105: offload bridge port flags to device drivers/net/dsa/b53/b53_common.c | 20 +- drivers/net/dsa/mv88e6xxx/chip.c | 21 +- drivers/net/dsa/ocelot/felix.c| 10 + drivers/net/dsa/sja1105/sja1105.h | 2 + drivers/net/dsa/sja1105/sja1105_main.c| 212 +- drivers/net/dsa/sja1105/sja1105_spi.c | 6 + .../marvell/prestera/prestera_switchdev.c | 54 +++-- .../mellanox/mlxsw/spectrum_switchdev.c | 90 drivers/net/ethernet/mscc/ocelot.c| 72 +- drivers/net/ethernet/mscc/ocelot_net.c| 7 +- drivers/net/ethernet/rocker/rocker.h | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 24 +- drivers/net/ethernet/rocker/rocker_ofdpa.c| 26 ++- drivers/net/ethernet/ti/cpsw_switchdev.c | 35 ++- drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 43 ++-- include/net/dsa.h | 7 +- include/net/switchdev.h | 14 +- include/soc/mscc/ocelot.h | 18 +- net/bridge/br_netlink.c | 162 ++--- net/bridge/br_private.h | 6 +- net/bridge/br_switchdev.c | 33 ++- net/bridge/br_sysfs_if.c | 21 +- net/dsa/dsa_priv.h| 8 +- net/dsa/port.c| 76 --- net/dsa/slave.c | 10 +- net/switchdev/switchdev.c | 11 +- 26 files changed, 654 insertions(+), 336 deletions(-) -- 2.25.1
