Re: [PATCH] bridge: extend BR_ISOLATE to full split-horizon

On 09/12/2021 18:23, Nikolay Aleksandrov wrote:

On 09/12/2021 18:08, Nikolay Aleksandrov wrote:

quoted

On 09/12/2021 17:42, Jakub Kicinski wrote:

quoted

On Thu,  9 Dec 2021 13:14:32 +0100 David Lamparter wrote:

quoted

Split-horizon essentially just means being able to create multiple
groups of isolated ports that are isolated within the group, but not
with respect to each other.

The intent is very different, while isolation is a policy feature,
split-horizon is intended to provide functional "multiple member ports
are treated as one for loop avoidance."  But it boils down to the same
thing in the end.

Signed-off-by: David Lamparter <redacted>
Cc: Nikolay Aleksandrov <redacted>
Cc: Alexandra Winter <wintera@linux.ibm.com>

Does not apply to net-next, you'll need to repost even if the code is
good. Please put [PATCH net-next] in the subject.

Hi,
For some reason this patch didn't make it to my inbox.. Anyway I was
able to see it now online, a few comments (sorry can't do them inline due
to missing mbox patch):
- please drop the sysfs part, we're not extending sysfs anymore
- split the bridge change from the driver
- drop the /* BR_ISOLATED - previously BIT(16) */ comment
- [IFLA_BRPORT_HORIZON_GROUP] = NLA_POLICY_MIN(NLA_S32, 0), why not just { .type = NLA_U32 } ?

quoted

- just forbid having both set (tb[IFLA_BRPORT_ISOLATED] && tb[IFLA_BRPORT_HORIZON_GROUP])
  user-space should use just one of the two, if isolated is set then it overwrites any older
  IFLA_BRPORT_HORIZON_GROUP settings, that should simplify things considerably

Actually they'll have to be exported together always... that's unfortunate. I get
why you need the extra netlink logic, I think we'll have to keep it.

So one relatively simple way to drop most of the logic is to have BRPORT_HORIZON_GROUP's
attribute get set after ISOLATED so it always overwrites it, which is similar to the current
situation but if you set only ISOLATED later it will function as intended (i.e. drop the logic
for horizon_group when using the old attr). Still will have to disallow ISOLATED = 0/HORIZON_GROUP >0
and ISOLATED=1/HORIZON_GROUP=0 though as these don't make sense.

e.g.:
if (tb[IFLA_BRPORT_ISOLATED])
	p->horizon_group = !!nla_get_u8(tb[IFLA_BRPORT_ISOLATED]);
if (tb[IFLA_BRPORT_HORIZON_GROUP])
	p->horizon_group = nla_get_u32(tb[IFLA_BRPORT_HORIZON_GROUP]);



This will be also in line with how they're exported (ISOLATED = 1 and HORIZON_GROUP >0).