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

On 09.12.21 17:08, Nikolay Aleksandrov wrote:

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 } ?
- 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

Yes, please keep it compatible with userspace setting IFLA_BRPORT_ISOLATED only.

Why the limitation (UAPI limited to positive signed int. (recommended ifindex namespace)) ?
You have the full unsigned space available, user-space can use it as it sees fit.
You can just remove the comment about recommended ifindex.

Also please extend the port isolation self-test with a test for a different horizon group.

Thanks,
 Nik