Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 07:27:24PM +0000, Vladimir Oltean wrote:

On Fri, Dec 10, 2021 at 08:10:21PM +0100, Ansuel Smith wrote:

quoted

quoted

Ok I added more tracing and packet are received to the tagger right
after the log from ipv6 "link becomes ready". That log just check if the
interface is up and if it does have a valid sched.
I notice after link becomes ready we have a CHANGE event for eth0. That
should be the correct way to understand when the cpu port is actually
usable.
(just to make it clear before the link becomes ready no packet is
received to the tagger and the completion timeouts)

-- 
	Ansuel

Sorry for the triple message spam... I have a solution. It seems packet
are processed as soon as dev_activate is called (so a qdisk is assigned)
By adding another bool like master_oper_ready and

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up);

static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
                                        struct net_device *master)
{
       struct dsa_notifier_master_state_info info;
       struct dsa_port *cpu_dp = master->dsa_ptr;

       info.master = master;
       info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;

       dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
}

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up)
{
       struct dsa_port *cpu_dp = master->dsa_ptr;
       bool notify = false;

       if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
           (cpu_dp->master_oper_ready && up))
               notify = true;

       cpu_dp->master_oper_ready = up;

       if (notify)
               dsa_tree_master_state_change(dst, master);
}

In slave.c at the NETDEV_CHANGE event the additional
dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
we have no timeout function. I just tested this and it works right away.

Think we need this additional check to make sure the tagger can finally
accept packet from the switch.

With this added I think this is ready.

Why ingress_queue?
I was looking at dev_activate() too, especially since net/ipv6/addrconf.c uses:

/* Check if link is ready: is it up and is a valid qdisc available */
static inline bool addrconf_link_ready(const struct net_device *dev)
{
	return netif_oper_up(dev) && !qdisc_tx_is_noop(dev);
}

and you can see that qdisc_tx_is_noop() checks for the qdisc on TX
queues, not ingress qdisc (which makes more sense anyway).

Anyway the reason why I didn't say anything about this is because I
don't yet understand how it is supposed to work. Specifically:

rtnl_lock

dev_open()
-> __dev_open()
   -> dev->flags |= IFF_UP;
   -> dev_activate()
      -> transition_one_qdisc()
-> call_netdevice_notifiers(NETDEV_UP, dev);

rtnl_unlock

so the qdisc should have already transitioned by the time NETDEV_UP is
emitted.

and since we already require a NETDEV_UP to have occurred, or dev->flags
to contain IFF_UP, I simply don't understand the following
(a) why would the qdisc be noop when we catch NETDEV_UP
(b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
    qdisc changes on a TX queue? If no one, then I'm not sure how we can
    reliably check for the state of the qdisc if we aren't notified
    about changes to it.

The ipv6 check is just a hint. The real clue was the second
NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
That is the one that calls the CHANGE event before the ready stuff.

I had problem tracking this as the change logic is "emit CHANGE when flags
change" but netdev_state_change is also called for other reason and one
example is dev_activate/dev_deactivate from linkwatch_do_dev.
It seems a bit confusing that a generic state change is called even when
flags are not changed and because of this is a bit problematic track why
the CHANGE event was called.

Wonder if linkwatch_do_dev should be changed and introduce a flag? But
that seems problematic if for whatever reason a driver use the CHANGE
event to track exactly dev_activate/deactivate.

-- 
	Ansuel