by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries

On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote:
> On Mon, Feb 06, 2023 at 17:02, Simon Horman <[email protected]> wrote:
> >
> > Just to clarify my suggestion one last time, it would be along the lines
> > of the following (completely untested!). I feel that it robustly covers
> > all cases for fdb_flags. And as a bonus doesn't need to be modified
> > if other (unsupported) flags are added in future.
> >
> > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > return -EOPNOTSUPP;
> >
> > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC)
> > if (is_dynamic)
> > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
> >
> >
> > And perhaps for other drivers:
> >
> > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > return -EOPNOTSUPP;
> > if (fdb_flags)
> > return 0;
> >
> > Perhaps a helper would be warranted for the above.
>
> How would such a helper look? Inline function is not clean.
>
> >
> > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP
> > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha
> > propagate, -EOPNOTSUPP.
>
> I looked at that, but changing the caller is also a bit ugly.

Answering on behalf of Simon, and hoping he will agree.

You are missing a big opportunity to make the kernel avoid doing useless work.
The dsa_slave_fdb_event() function runs in atomic switchdev notifier context,
and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable
context to program hardware.

Only that scheduling a deferred work item is not exactly cheap, so we try to
avoid doing that unless we know that we'll end up doing something with that
FDB entry once the deferred work does get scheduled:

/* Check early that we're not doing work in vain.
* Host addresses on LAG ports still require regular FDB ops,
* since the CPU port isn't in a LAG.
*/
if (dp->lag && !host_addr) {
if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
return -EOPNOTSUPP;
} else {
if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del)
return -EOPNOTSUPP;
}

What you should be doing is you should be using the pahole tool to find
a good place for a new unsigned long field in struct dsa_switch, and add
a new field ds->supported_fdb_flags. You should extend the early checking
from dsa_slave_fdb_event() and exit without doing anything if the
(fdb->flags & ~ds->supported_fdb_flags) expression is non-zero.

This way you would kill 2 birds with 1 stone, since individual drivers
would no longer need to check the flags; DSA would guarantee not calling
them with unsupported flags.

It would be also very good to reach an agreement with switchdev
maintainers regarding the naming of the is_static/is_dyn field.

It would also be excellent if you could rename "fdb_flags" to just
"flags" within DSA.

2023-02-20 14:12:56

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries

On Fri, Feb 17, 2023 at 07:44:31PM +0200, Vladimir Oltean wrote:
> On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote:
> > On Mon, Feb 06, 2023 at 17:02, Simon Horman <[email protected]> wrote:
> > >
> > > Just to clarify my suggestion one last time, it would be along the lines
> > > of the following (completely untested!). I feel that it robustly covers
> > > all cases for fdb_flags. And as a bonus doesn't need to be modified
> > > if other (unsupported) flags are added in future.
> > >
> > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > > return -EOPNOTSUPP;
> > >
> > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC)
> > > if (is_dynamic)
> > > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
> > >
> > >
> > > And perhaps for other drivers:
> > >
> > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > > return -EOPNOTSUPP;
> > > if (fdb_flags)
> > > return 0;
> > >
> > > Perhaps a helper would be warranted for the above.
> >
> > How would such a helper look? Inline function is not clean.
> >
> > >
> > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP
> > > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha
> > > propagate, -EOPNOTSUPP.
> >
> > I looked at that, but changing the caller is also a bit ugly.
>
> Answering on behalf of Simon, and hoping he will agree.

Sorry for not responding earlier - I was on vacation last week.

TBH my idea was not nearly as well developed as the one you describe below.
But yes, I agree this is an interesting approach.

> You are missing a big opportunity to make the kernel avoid doing useless work.
> The dsa_slave_fdb_event() function runs in atomic switchdev notifier context,
> and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable
> context to program hardware.
>
> Only that scheduling a deferred work item is not exactly cheap, so we try to
> avoid doing that unless we know that we'll end up doing something with that
> FDB entry once the deferred work does get scheduled:
>
> /* Check early that we're not doing work in vain.
> * Host addresses on LAG ports still require regular FDB ops,
> * since the CPU port isn't in a LAG.
> */
> if (dp->lag && !host_addr) {
> if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
> return -EOPNOTSUPP;
> } else {
> if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del)
> return -EOPNOTSUPP;
> }
>
> What you should be doing is you should be using the pahole tool to find
> a good place for a new unsigned long field in struct dsa_switch, and add
> a new field ds->supported_fdb_flags. You should extend the early checking
> from dsa_slave_fdb_event() and exit without doing anything if the
> (fdb->flags & ~ds->supported_fdb_flags) expression is non-zero.
>
> This way you would kill 2 birds with 1 stone, since individual drivers
> would no longer need to check the flags; DSA would guarantee not calling
> them with unsupported flags.
>
> It would be also very good to reach an agreement with switchdev
> maintainers regarding the naming of the is_static/is_dyn field.
>
> It would also be excellent if you could rename "fdb_flags" to just
> "flags" within DSA.