Message-ID: <20240419124223.2388295d@wsk>
Date: Fri, 19 Apr 2024 12:42:23 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Casper Andersson <casper.casan@...il.com>
Cc: netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>, Andrew Lunn
 <andrew@...n.ch>, Eric Dumazet <edumazet@...gle.com>, Vladimir Oltean
 <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>, Jakub
 Kicinski <kuba@...nel.org>, Oleksij Rempel <o.rempel@...gutronix.de>,
 Tristram.Ha@...rochip.com, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>, Ravi Gunasekaran <r-gunasekaran@...com>, Simon
 Horman <horms@...nel.org>, Nikita Zhandarovich <n.zhandarovich@...tech.ru>,
 Murali Karicheri <m-karicheri2@...com>, Jiri Pirko <jiri@...nulli.us>, Dan
 Carpenter <dan.carpenter@...aro.org>, Ziyang Xuan
 <william.xuanziyang@...wei.com>, Shigeru Yoshida <syoshida@...hat.com>,
 "Ricardo B. Marliere" <ricardo@...liere.net>, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support
 (HSR-SAN)

Hi Casper,

> On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
> Hi Lukasz,
> 
> > Hi Casper,
> >  
> >> Hi,
> >> 
> >> Sorry for the late reply, I was awaiting confirmation on what I can
> >> say about the hardware I have access to. They won't let me say the
> >> name :( but I can give some details.  
> >
> > Ok, good :-)
> >
> > At least I'm not alone and there is another person who can validate
> > the code (or behaviour) on another HSR HW.
> >
> > (Some parts of the specification could be double checked on another
> > HW as well).
> >  
> >> 
> >> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:  
> >> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:    
> >> >> > Changes for v3:
> >> >> >
> >> >> > - Modify frame passed Port C (Interlink) to have RedBox's
> >> >> > source address (SA) This fixes issue with connecting L2
> >> >> > switch to Interlink Port as switches drop frames with SA
> >> >> > other than one registered in their (internal) routing tables.
> >> >> >      
> >> >>     
> >> >> > +	/* When HSR node is used as RedBox - the frame
> >> >> > received from HSR ring
> >> >> > +	 * requires source MAC address (SA) replacement to
> >> >> > one which can be
> >> >> > +	 * recognized by SAN devices (otherwise, frames are
> >> >> > dropped by switch)
> >> >> > +	 */
> >> >> > +	if (port->type == HSR_PT_INTERLINK)
> >> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> >> >> > +
> >> >> > port->hsr->macaddress_redbox); 
> >> >> 
> >> >> I'm not really understanding the reason for this change. Can you
> >> >> explain it in more detail?    
> >> >
> >> > According to the HSR standard [1] the RedBox device shall work
> >> > as a "proxy" [*] between HSR network and SAN (i.e. "normal"
> >> > ethernet) devices.
> >> >
> >> > This particular snippet handles the situation when frame from HSR
> >> > node is supposed to be sent to SAN network. In that case the SA
> >> > of HSR (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC
> >> > address of RedBox is known and used by SAN devices.
> >> >
> >> >
> >> > Node A  hsr1  |======| hsr1 Node Redbox |   |
> >> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> >> > 	      |      |        	 (SA_RB)|   |  (e.g switch)
> >> >
> >> >
> >> > (the ====== represents duplicate link - like lan1,lan2)
> >> >
> >> > If the SA_A would be passed to SAN (e.g. switch) the switch could
> >> > get confused as also RedBox MAC address would be used. Hence, all
> >> > the frames going out from "Node Redbox" have SA set to SA_RB.
> >> >
> >> > According to [1] - RedBox shall have the MAC address.
> >> > This is similar to problem from [2].    
> >> 
> >> Thanks for the explanation, but I still don't quite follow in what
> >> way the SAN gets confused. "also RedBox MAC address would be
> >> used", when does this happen? Do you mean that some frames from
> >> Node A end up using the RedBox MAC address so it's best if they
> >> all do?  
> >
> > The SAN (let's say it is a switch) can communicate with RedBox or
> > Node A. In that way the DA is different for both (so SA on reply is
> > also different). On my setup I've observed frames drop (caused
> > probably by switch filtering of incoming traffic not matching the
> > outgoing one).
> >
> > When I only use SA of RedBox on traffic going to SAN, the problem is
> > gone.
> >
> > IMHO, such separation (i.e. to use only RedBox's SA on traffic
> > going to SAN) is the "proxy" mentioned in the standard.
> >  
> >> 
> >> I see there is already some address replacement going on in the HSR
> >> interface, as you pointed out in [2]. And I get your idea of being
> >> a proxy. If no one else is opposed to this then I'm fine with it
> >> too. 
> >
> > Ok.
> >  
> >> >> The standard does not say to modify the
> >> >> SA. However, it also does not say to *not* modify it in HSR-SAN
> >> >> mode like it does in other places. In HSR-HSR and HSR-PRP mode
> >> >> modifying SA breaks the duplicate discard.    
> >> >
> >> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two
> >> > types (and not fully compatible) networks.
> >> >    
> >> >> So keeping the same behavior for all
> >> >> modes would be ideal.
> >> >> 
> >> >> I imagine any HW offloaded solutions will not modify the SA, so
> >> >> if possible the SW should also behave as such.    
> >> >
> >> > The HW offloading in most cases works with HSR-HSR setup (i.e. it
> >> > duplicates frames automatically or discards them when recived -
> >> > like ksz9477 [3]).
> >> >
> >> > I think that RedBox HW offloading would be difficult to achieve
> >> > to comply with standard. One "rough" idea would be to configure
> >> > aforementioned ksz9477 to pass all frames in its HW between SAN
> >> > and HSR network (but then it wouldn't filter them).    
> >> 
> >> I don't know anything about ksz9477. The hardware I have access to
> >> is supposed to be compliant with 2016 version in an offloaded
> >> situation for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).  
> >
> > Hmm... Interesting.
> >
> > As fair as I know - the ksz9477 driver from Microchip for RedBox
> > sets internal (i.e. in chip) vlan for Node_A, Node_B and Interlink,
> > so _all_ packets are flowing back and forth between HSR and SAN
> > networks .... 
> >> Though, I haven't
> >> verified if the operation is fully according to standard.  
> >
> > You may use wireshark on device connected as SAN to redbox and then
> > see if there are any frames (especially supervisory ones) passed
> > from HSR network.  
> 
> I realized I should clarify, what I'm running is non-upstream
> software.

Ok.

> And by offloaded I mean the redbox forwarding is
> offloaded. Supervision frames are still handled in SW and only sent on
> HSR/PRP ports, and doesn't reach any SAN nodes. Basic operation works
> as it should.

Ok.

> 
> >> It does not
> >> modify any addresses in HW.  
> >
> > By address - you mean the MAC addresses of nodes?  
> 
> I mean that it forwards all frames without modification (except
> HSR/PRP and VLAN tags). It does not update SMAC with the proxy MAC
> like your implementation does.

Hmm... I'm wondering how "proxy" is implemented then.
Also, what is the purpose of ProxyNodeTable in that case?

> 
> >> Does the interlink port also reach the drivers?  
> >
> > Could you be more specific in your question?  
> 
> Sorry, it was connected to the question below if it sets anything up
> in the drivers for the interlink port. And you answered it.

Ok.

> 
> >> Does it call
> >> port_hsr_join and try to join as an HSR port?   
> >
> > No, not yet.
> >
> > The community (IIRC Vladimir Oltean) suggested to first implement
> > the RedBox Interlink (HSR-SAN) in SW. Then, we may think about
> > adding offloading support for it.
> >  
> >> Do we maybe need a
> >> separate path or setting for configuring the interlink in the
> >> different modes (SAN, HSR, PRP interlink)?  
> >
> > I think that it shall be handled as an extra parameter (like we do
> > have now with 'supervision' or 'version') in ip link add.
> >
> > However, first I would like to have the "interlink" parameter added
> > to iproute2 and then we can extend it to other modes if requred.  
> 
> Alright, doing SW implementation first sounds good. From userspace it
> can probably be an extra parameter. But for the driver configuration
> maybe we want a port_interlink_join? (when it comes to implementing
> that).

IMHO, having port_interlink_join() may be useful in the future to
provide offloading support.

> 
> 
> I did some testing with veth interfaces (everything in SW) with your
> patches. I tried to do a setup like yours
>                 
>                   +-vethA---vethB-+
>                   |               |
> vethF---vethE---hsr0             hsr1
>                   |               |
>                   +-vethC---vethD-+
> 
> Sending traffic from vethF results in 3 copies being seen on the ring
> ports. One of which ends up being forwarded back to vethF (with SMAC
> updated to the proxy address). I assume this is not intended behavior.

I've reported this [2] (i.e. duplicated packets on HSR network with
veth) when I was checking hsr_ping.sh [1] script for regression.

(However, I don't see the DUP pings on my KSZ9477 setup).

> 
> 
> Setup:
> ip link add dev vethA type veth peer name vethB
> ip link add dev vethC type veth peer name vethD
> ip link add dev vethE type veth peer name vethF
> ip link set up dev vethA
> ip link set up dev vethB
> ip link set up dev vethC
> ip link set up dev vethD
> ip link set up dev vethE
> ip link set up dev vethF
> 
> ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink
> vethE supervision 45 version 1 ip link add name hsr1 type hsr slave1
> vethB slave2 vethD supervision 45 version 1 ip link set dev hsr0 up
> ip link set dev hsr1 up
> 
> I used Nemesis to send random UDP broadcast packets but you could use
> whatever: nemesis udp -d vethF -c 10000 -i 1 

Ok, I will check nemesis load as well.

Can you check the hsr_redbox.sh (from this patch set) and hsr_ping.sh ?

> 
> BR,
> Casper

Links:

[1] -
https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/net/hsr/hsr_ping.sh

[2] -
https://lore.kernel.org/linux-kernel/20240418125336.7305d545@wsk/T/#m9c54a1a31366e4d1caec8fceb4329c5dbe9cc9aa


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists