Re: [PATCH net-next] net: ocelot: Extend MRP

[Horatiu Vultur]

The 03/11/2021 20:02, Vladimir Oltean wrote:
> 
> On Thu, Mar 11, 2021 at 08:30:08PM +0100, Horatiu Vultur wrote:
> > > > +static void ocelot_mrp_save_mac(struct ocelot *ocelot,
> > > > +                             struct ocelot_port *port)
> > > > +{
> > > > +     ocelot_mact_learn(ocelot, PGID_MRP, mrp_test_dmac,
> > > > +                       port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
> > > > +     ocelot_mact_learn(ocelot, PGID_MRP, mrp_control_dmac,
> > > > +                       port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
> > >
> > > Let me make sure I understand.
> > > By learning these multicast addresses, you mark them as 'not unknown' in
> > > the MAC table, because otherwise they will be flooded, including to the
> > > CPU port module, and there's no way you can remove the CPU from the
> > > flood mask, even if the packets get later redirected through VCAP IS2?
> >
> > Yes, so far you are right.
> >
> > > I mean that's the reason why we have the policer on the CPU port for the
> > > drop action in ocelot_vcap_init, no?
> >
> > I am not sure that would work because I want the action to be redirect
> > and not policy. Or maybe I am missing something?
> 
> Yes, it is not the same context as for tc-drop. The problem for tc-drop
> was that the packets would get removed from the hardware datapath, but
> they would still get copied to the CPU nonetheless. A policer there was
> an OK solution because we wanted to kill those packets completely. Here,
> the problem is the same, but we cannot use the same solution, since a
> policer will also prevent the frames from being redirected.
> 
> > >
> > > > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > > > index 425ff29d9389..c41696d2e82b 100644
> > > > --- a/include/soc/mscc/ocelot.h
> > > > +++ b/include/soc/mscc/ocelot.h
> > > > @@ -51,6 +51,7 @@
> > > >   */
> > > >
> > > >  /* Reserve some destination PGIDs at the end of the range:
> > > > + * PGID_MRP: used for not flooding MRP frames to CPU
> > >
> > > Could this be named PGID_BLACKHOLE or something? It isn't specific to
> > > MRP if I understand correctly. We should also probably initialize it
> > > with zero.
> >
> > It shouldn't matter the value, what is important that the CPU port not
> > to be set. Because the value of this PGID will not be used in the
> > fowarding decision.
> > Currently only MRP is using it so that is the reason for naming it like
> > that but I can rename it and initialized it to 0 to be more clear.
> 
> So tell me more about this behavior.
> Is there no way to suppress the flooding to CPU action, even if the
> frame was hit by a TCAM rule? Let's forget about MRP, assume this is an
> broadcast IPv4 packet, and we have a matching src_ip rule to perform
> mirred egress redirect to another port.
> Would the CPU be flooded with this traffic too? What would you do to
> avoid that situation?

I think so, I need to ask around to be able to answer your question.

You have to think about CPU port as a special port. If at any point
while the frame goes through the switch, there is a decision to copy the
frame to CPU, the frame will be copied to CPU regardless of the previous
or next decisions. That is at least my understanding.

-- 
/Horatiu