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

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 09/12/2021 17:42, Jakub Kicinski wrote:
> On Thu,  9 Dec 2021 13:14:32 +0100 David Lamparter wrote:
>> 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 <equinox@diac24.net>
>> Cc: Nikolay Aleksandrov <nikolay@nvidia.com>
>> 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

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