Message-ID: <20221122113412.dg4diiu5ngmulih2@skbuf>
Date:   Tue, 22 Nov 2022 13:34:28 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Steve Williams <steve.williams@...cruise.com>,
        netdev@...r.kernel.org, vinicius.gomes@...el.com,
        xiaoliang.yang_1@....com
Subject: Re: [PATCH net-next] net/hanic: Add the hanic network interface for
 high availability links

On Mon, Nov 21, 2022 at 07:58:10PM -0800, Jakub Kicinski wrote:
> On Fri, 18 Nov 2022 15:26:39 -0800 Steve Williams wrote:
> > This is a virtual device that implements support for 802.1cb R-TAGS
> > and duplication and deduplication. The hanic nic itself is not a device,
> > but enlists ethernet nics to act as parties in a high-availability
> > link. Outbound packets are duplicated and tagged with R-TAGs, then
> > set out the enlisted links. Inbound packets with R-TAGs have their
> > R-TAGs removed, and duplicates are dropped to complete the link. The
> > algorithm handles links being completely disconnected, sporadic packet
> > loss, and out-of-order arrivals.
> >
> > To the extent possible, the link is self-configuring: It detects and
> > brings up streams as R-TAG'ed packets are detected, and creates streams
> > for outbound packets unless explicitly filtered to skip tagging.
>
> Superficially pattern matching on the standard - there has been
> a discussion about 802.1cb support in the HW offload context:
>
> https://lore.kernel.org/netdev/20210928114451.24956-1-xiaoliang.yang_1@nxp.com/
>
> Would be great if the two effort could align.

Thanks, Jakub, I hadn't noticed Steve's patch.

I have some problems getting it to compile with W=1 C=1 (possibly not only
with those flags).

  CHECK   ../drivers/net/hanic/hanic_dev.c
../drivers/net/hanic/hanic_dev.c:70:16: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:106:29: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:109:22: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:128:29: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:242:19: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:295:29: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:510:54: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:596:22: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:639:39: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:640:39: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:655:47: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:656:47: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:753:42: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:754:42: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:776:20: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:800:34: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_dev.c:802:33: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_netns.c:14:14: warning: symbol 'hanic_net_id' was not declared. Should it be static?
../drivers/net/hanic/hanic_filter.c:70:42: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:77:21: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:92:42: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:99:21: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:115:21: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:127:21: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:160:78: warning: Using plain integer as NULL pointer
../drivers/net/hanic/hanic_filter.c:161:34: warning: Using plain integer as NULL pointer
In file included from ../include/linux/kobject.h:20,
                 from ../include/linux/energy_model.h:7,
                 from ../include/linux/device.h:16,
                 from ../drivers/net/hanic/hanic_priv.h:10,
                 from ../drivers/net/hanic/hanic_sysfs.c:7:
../drivers/net/hanic/hanic_sysfs.c: In function ‘hanic_init_sysfs’:
../drivers/net/hanic/hanic_sysfs.c:83:31: error: ‘struct hanic_netns’ has no member named ‘class_attr_sandlan_interfaces’; did you mean ‘class_attr_hanic_interfaces’?
   83 |         sysfs_attr_init(&xns->class_attr_sandlan_interfaces.attr);
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../include/linux/sysfs.h:55:10: note: in definition of macro ‘sysfs_attr_init’
   55 |         (attr)->key = &__key;                           \
      |          ^~~~
In file included from ../include/linux/kobject.h:20,
                 from ../include/linux/energy_model.h:7,
                 from ../include/linux/device.h:16,
                 from ../drivers/net/hanic/hanic_priv.h:10,
                 from ../drivers/net/hanic/hanic_streams.c:11:
../drivers/net/hanic/hanic_streams.c: In function ‘hanic_map_stream’:
../include/linux/sysfs.h:55:15: error: ‘struct device_attribute’ has no member named ‘key’
   55 |         (attr)->key = &__key;                           \
      |               ^~
../drivers/net/hanic/hanic_streams.c:90:17: note: in expansion of macro ‘sysfs_attr_init’
   90 |                 sysfs_attr_init(&stream->attr);
      |                 ^~~~~~~~~~~~~~~


It will take me a while until I come with more intelligent feedback, but
as a first set of questions (based solely on the documentation and not
the code, I'm wondering a few things):

1. You seem to create a usage model which is heavily optimized for ping
   (the termination plane), but not at all optimized for the forwarding
   plane. What I mean is that the documentation says "Inbound traffic
   that does not have an R-TAG is assumed to not be redundant, and is
   simply passed up the network stack." That's a pretty big limitation,
   isn't that so? If you want to build a RED box (intermediary device
   which connects a non-redundant device into a redundant network) out
   of a Linux device with hanic, how would you do that? Inbound traffic
   which comes from the FRER-unaware device must match on a TSN stream
   which says where it should go and how it should be tagged. And the
   set of destination ports for inbound traffic may well be a subset of
   the other enlisted ports, not the hanic device or one of its VLAN
   uppers.

2. What about stream identification rules which aren't based on MAC DA/VLAN?
   How about MAC SA/VLAN, or SIP, DIP, or active identification rules,
   or generic byte@...set pattern matching?

3. Shouldn't TSN streams be input by NETCONF/YANG in a useful industrial
   production network, rather than be auto-discovered based on incoming
   and outgoing traffic?

I mean, I can truly, genuinely understand why you made some of the
choices you've made in the design of this driver, but the more I look at
Xiaoliang's tc filter/action based take, the more I get the feeling that
his approach is the way to fully exploit what can be accomplished with
the 802.1CB standard. What you're presenting is more like your take on a
subset that's useful to you (I mean, it *is* called "Cruise High
Availability NIC driver", so at least it's truthful to that).

Powered by blists - more mailing lists