diff mbox series

[net-next,1/5] net: bridge: add dynamic flag to switchdev notifier

Message ID 20230130173429.3577450-2-netdev@kapio-technology.com
State New
Headers
Series ATU and FDB synchronization on locked ports |

Commit Message

Hans Schultz Jan. 30, 2023, 5:34 p.m. UTC 
  To be able to add dynamic FDB entries to drivers from userspace, the
dynamic flag must be added when sending RTM_NEWNEIGH events down.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 include/net/switchdev.h   | 1 +
 net/bridge/br_switchdev.c | 2 ++
 2 files changed, 3 insertions(+)

Comments

Ido Schimmel Feb. 1, 2023, 6:10 p.m. UTC | #1 
On Mon, Jan 30, 2023 at 06:34:25PM +0100, Hans J. Schultz wrote:
> To be able to add dynamic FDB entries to drivers from userspace, the
> dynamic flag must be added when sending RTM_NEWNEIGH events down.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> ---
>  include/net/switchdev.h   | 1 +
>  net/bridge/br_switchdev.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index ca0312b78294..aaf918d4ba67 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
>  	u8 added_by_user:1,
>  	   is_local:1,
>  	   locked:1,
> +	   is_dyn:1,
>  	   offloaded:1;
>  };
>  
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 7eb6fd5bb917..4420fcbbfdb2 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -136,6 +136,8 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
>  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> +	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags) &&

Why not 'is_static' and be consistent with the bridge flag like all the
other fields?

Regardless of how you name this field, it is irrelevant for
'SWITCHDEV_FDB_ADD_TO_BRIDGE' notifications that all add FDB entries
with the 'BR_FDB_ADDED_BY_EXT_LEARN' flag set, which makes
'BR_FDB_STATIC' irrelevant.

> +		item->added_by_user;

Unclear why this is needed...

>  	item->locked = false;
>  	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>  	item->info.ctx = ctx;
> -- 
> 2.34.1
>
Hans Schultz Feb. 2, 2023, 7:28 a.m. UTC | #2 
On 2023-02-01 19:10, Ido Schimmel wrote:
> On Mon, Jan 30, 2023 at 06:34:25PM +0100, Hans J. Schultz wrote:
>> To be able to add dynamic FDB entries to drivers from userspace, the
>> dynamic flag must be added when sending RTM_NEWNEIGH events down.
>> 
>> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
>> ---
>>  include/net/switchdev.h   | 1 +
>>  net/bridge/br_switchdev.c | 2 ++
>>  2 files changed, 3 insertions(+)
>> 
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index ca0312b78294..aaf918d4ba67 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
>>  	u8 added_by_user:1,
>>  	   is_local:1,
>>  	   locked:1,
>> +	   is_dyn:1,
>>  	   offloaded:1;
>>  };
>> 
>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
>> index 7eb6fd5bb917..4420fcbbfdb2 100644
>> --- a/net/bridge/br_switchdev.c
>> +++ b/net/bridge/br_switchdev.c
>> @@ -136,6 +136,8 @@ static void br_switchdev_fdb_populate(struct 
>> net_bridge *br,
>>  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>>  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>>  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
>> +	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> 
> Why not 'is_static' and be consistent with the bridge flag like all the
> other fields?
> 
> Regardless of how you name this field, it is irrelevant for
> 'SWITCHDEV_FDB_ADD_TO_BRIDGE' notifications that all add FDB entries
> with the 'BR_FDB_ADDED_BY_EXT_LEARN' flag set, which makes
> 'BR_FDB_STATIC' irrelevant.
> 
>> +		item->added_by_user;
> 
> Unclear why this is needed...
> 

The answer to those two questions lies in my earlier correspondences 
(with Oltean) on the RFC version.
Ido Schimmel Feb. 2, 2023, 4:11 p.m. UTC | #3 
On Thu, Feb 02, 2023 at 08:28:36AM +0100, netdev@kapio-technology.com wrote:
> On 2023-02-01 19:10, Ido Schimmel wrote:
> > On Mon, Jan 30, 2023 at 06:34:25PM +0100, Hans J. Schultz wrote:
> > > To be able to add dynamic FDB entries to drivers from userspace, the
> > > dynamic flag must be added when sending RTM_NEWNEIGH events down.
> > > 
> > > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> > > ---
> > >  include/net/switchdev.h   | 1 +
> > >  net/bridge/br_switchdev.c | 2 ++
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> > > index ca0312b78294..aaf918d4ba67 100644
> > > --- a/include/net/switchdev.h
> > > +++ b/include/net/switchdev.h
> > > @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
> > >  	u8 added_by_user:1,
> > >  	   is_local:1,
> > >  	   locked:1,
> > > +	   is_dyn:1,
> > >  	   offloaded:1;
> > >  };
> > > 
> > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> > > index 7eb6fd5bb917..4420fcbbfdb2 100644
> > > --- a/net/bridge/br_switchdev.c
> > > +++ b/net/bridge/br_switchdev.c
> > > @@ -136,6 +136,8 @@ static void br_switchdev_fdb_populate(struct
> > > net_bridge *br,
> > >  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
> > >  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
> > >  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> > > +	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> > 
> > Why not 'is_static' and be consistent with the bridge flag like all the
> > other fields?
> > 
> > Regardless of how you name this field, it is irrelevant for
> > 'SWITCHDEV_FDB_ADD_TO_BRIDGE' notifications that all add FDB entries
> > with the 'BR_FDB_ADDED_BY_EXT_LEARN' flag set, which makes
> > 'BR_FDB_STATIC' irrelevant.
> > 
> > > +		item->added_by_user;
> > 
> > Unclear why this is needed...
> > 
> 
> The answer to those two questions lies in my earlier correspondences (with
> Oltean) on the RFC version.

It is not up to me as a reviewer to dig up old versions of the patch and
find out what was changed and why. It is up to you as the submitter of
the patch to provide all this information in the patch posting. Please
read:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Specifically:

"Review comments or questions that do not lead to a code change should
almost certainly bring about a comment or changelog entry so that the
next reviewer better understands what is going on."

And:

"Other comments relevant only to the moment or the maintainer, not
suitable for the permanent changelog, should also go here. A good
example of such comments might be patch changelogs which describe what
has changed between the v1 and v2 version of the patch.

Please put this information after the --- line which separates the
changelog from the rest of the patch. The version information is not
part of the changelog which gets committed to the git tree. It is
additional information for the reviewers."

Thanks
Hans Schultz Feb. 2, 2023, 4:38 p.m. UTC | #4 
On 2023-02-02 17:11, Ido Schimmel wrote:
> On Thu, Feb 02, 2023 at 08:28:36AM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2023-02-01 19:10, Ido Schimmel wrote:
>> > On Mon, Jan 30, 2023 at 06:34:25PM +0100, Hans J. Schultz wrote:
>> > > To be able to add dynamic FDB entries to drivers from userspace, the
>> > > dynamic flag must be added when sending RTM_NEWNEIGH events down.
>> > >
>> > > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
>> > > ---
>> > >  include/net/switchdev.h   | 1 +
>> > >  net/bridge/br_switchdev.c | 2 ++
>> > >  2 files changed, 3 insertions(+)
>> > >
>> > > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> > > index ca0312b78294..aaf918d4ba67 100644
>> > > --- a/include/net/switchdev.h
>> > > +++ b/include/net/switchdev.h
>> > > @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
>> > >  	u8 added_by_user:1,
>> > >  	   is_local:1,
>> > >  	   locked:1,
>> > > +	   is_dyn:1,
>> > >  	   offloaded:1;
>> > >  };
>> > >
>> > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
>> > > index 7eb6fd5bb917..4420fcbbfdb2 100644
>> > > --- a/net/bridge/br_switchdev.c
>> > > +++ b/net/bridge/br_switchdev.c
>> > > @@ -136,6 +136,8 @@ static void br_switchdev_fdb_populate(struct
>> > > net_bridge *br,
>> > >  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>> > >  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>> > >  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
>> > > +	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags) &&
>> >
>> > Why not 'is_static' and be consistent with the bridge flag like all the
>> > other fields?
>> >
>> > Regardless of how you name this field, it is irrelevant for
>> > 'SWITCHDEV_FDB_ADD_TO_BRIDGE' notifications that all add FDB entries
>> > with the 'BR_FDB_ADDED_BY_EXT_LEARN' flag set, which makes
>> > 'BR_FDB_STATIC' irrelevant.
>> >
>> > > +		item->added_by_user;
>> >
>> > Unclear why this is needed...
>> >
>> 
>> The answer to those two questions lies in my earlier correspondences 
>> (with
>> Oltean) on the RFC version.
> 
> It is not up to me as a reviewer to dig up old versions of the patch 
> and
> find out what was changed and why. It is up to you as the submitter of
> the patch to provide all this information in the patch posting. Please
> read:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> 
> Specifically:
> 
> "Review comments or questions that do not lead to a code change should
> almost certainly bring about a comment or changelog entry so that the
> next reviewer better understands what is going on."
> 
> And:
> 
> "Other comments relevant only to the moment or the maintainer, not
> suitable for the permanent changelog, should also go here. A good
> example of such comments might be patch changelogs which describe what
> has changed between the v1 and v2 version of the patch.
> 
> Please put this information after the --- line which separates the
> changelog from the rest of the patch. The version information is not
> part of the changelog which gets committed to the git tree. It is
> additional information for the reviewers."
> 
> Thanks


Sorry about that. I thought it would be easily found...

On the first question please look here:
https://lore.kernel.org/netdev/20230119134045.fqdt6zrna5x3iavt@skbuf/

On the second question it is what Oltean pointed out to me here...
https://lore.kernel.org/netdev/20230118230135.szu6a7kvt2mjb3i5@skbuf/

Oltean says there:
"This is not true, because it assumes that DSA never called 
port_fdb_add()
up until now for bridge FDB entries with the BR_FDB_STATIC flag unset,
which is incorrect (it did)."

Though as I see it, if it is only from the DSA layer on, the new 
is_dynamic flag would not be set anyway in the case he references. And 
as can be seen the change is in the bridge layer, as the rest is just 
propagating the flag, but it ensures that to set this flag that it comes 
from the user adding an FDB entry.
Ido Schimmel Feb. 3, 2023, 4:14 p.m. UTC | #5 
On Thu, Feb 02, 2023 at 05:38:06PM +0100, netdev@kapio-technology.com wrote:
> On the first question please look here:
> https://lore.kernel.org/netdev/20230119134045.fqdt6zrna5x3iavt@skbuf/

It seems Vladimir also wants the new field to be named 'is_static'
instead of 'is_dyn'. In your reason you mention
'SWITCHDEV_FDB_ADD_TO_BRIDGE', but this is not the interesting case for
the field. This event is used for devices to notify the bridge on new
learned entries. The bridge marks them as "extern_learn" which means
that "dynamic" / "static" flags are irrelevant.

The interesting case for the new field is the bridge to device direction
('SWITCHDEV_FDB_ADD_TO_DEVICE'). Drivers need to be patched to take the
new field into account when deciding the policy to program the entry
with. They can do it just as well if you name the new field 'is_static'
instead of 'is_dyn'.

> On the second question it is what Oltean pointed out to me here...
> https://lore.kernel.org/netdev/20230118230135.szu6a7kvt2mjb3i5@skbuf/
> 
> Oltean says there:
> "This is not true, because it assumes that DSA never called port_fdb_add()
> up until now for bridge FDB entries with the BR_FDB_STATIC flag unset,
> which is incorrect (it did)."
> 
> Though as I see it, if it is only from the DSA layer on, the new is_dynamic
> flag would not be set anyway in the case he references. And as can be seen
> the change is in the bridge layer, as the rest is just propagating the flag,
> but it ensures that to set this flag that it comes from the user adding an
> FDB entry.

OK, so can't this hunk:

```
	if (fdb_info->is_dyn)
		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
```

Become:

```
	if (fdb_info->is_dyn && !fdb_info->added_by_user)
		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
```

?

Then there is no need to fold 'added_by_user' into 'is_dyn' in the
bridge driver. I *think* this is the change Vladimir asked you to do.
Vladimir Oltean Feb. 3, 2023, 4:26 p.m. UTC | #6 
On Fri, 3 Feb 2023 at 18:14, Ido Schimmel <idosch@idosch.org> wrote:
> I *think* this is the change Vladimir asked you to do.

Yup, although instead of "is_dyn", I would still prefer "!is_static",
but again, that's a preference for bridge/switchdev maintainers to
override.
Hans Schultz Feb. 3, 2023, 4:27 p.m. UTC | #7 
On 2023-02-03 17:14, Ido Schimmel wrote:
> 
> OK, so can't this hunk:
> 
> ```
> 	if (fdb_info->is_dyn)
> 		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
> ```
> 
> Become:
> 
> ```
> 	if (fdb_info->is_dyn && !fdb_info->added_by_user)
> 		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
> ```
> 
> ?
> 
> Then there is no need to fold 'added_by_user' into 'is_dyn' in the
> bridge driver. I *think* this is the change Vladimir asked you to do.

I suppose you mean?:
  	if (fdb_info->is_dyn && fdb_info->added_by_user)
  		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
Ido Schimmel Feb. 3, 2023, 5:06 p.m. UTC | #8 
On Fri, Feb 03, 2023 at 05:27:43PM +0100, netdev@kapio-technology.com wrote:
> On 2023-02-03 17:14, Ido Schimmel wrote:
> > 
> > OK, so can't this hunk:
> > 
> > ```
> > 	if (fdb_info->is_dyn)
> > 		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
> > ```
> > 
> > Become:
> > 
> > ```
> > 	if (fdb_info->is_dyn && !fdb_info->added_by_user)
> > 		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;
> > ```
> > 
> > ?
> > 
> > Then there is no need to fold 'added_by_user' into 'is_dyn' in the
> > bridge driver. I *think* this is the change Vladimir asked you to do.
> 
> I suppose you mean?:
>  	if (fdb_info->is_dyn && fdb_info->added_by_user)
>  		fdb_flags |= DSA_FDB_FLAG_DYNAMIC;

Yes
diff mbox series

Patch

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index ca0312b78294..aaf918d4ba67 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -249,6 +249,7 @@  struct switchdev_notifier_fdb_info {
 	u8 added_by_user:1,
 	   is_local:1,
 	   locked:1,
+	   is_dyn:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 7eb6fd5bb917..4420fcbbfdb2 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -136,6 +136,8 @@  static void br_switchdev_fdb_populate(struct net_bridge *br,
 	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
 	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
 	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
+	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags) &&
+		item->added_by_user;
 	item->locked = false;
 	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
 	item->info.ctx = ctx;