Re: [PATCH v2 net-next 2/3] net: dsa: Make deferred_xmit private to sja1105

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/3/2020 4:37 PM, Vladimir Oltean wrote:
> There are 3 things that are wrong with the DSA deferred xmit mechanism:
> 
> 1. Its introduction has made the DSA hotpath ever so slightly more
>    inefficient for everybody, since DSA_SKB_CB(skb)->deferred_xmit needs
>    to be initialized to false for every transmitted frame, in order to
>    figure out whether the driver requested deferral or not (a very rare
>    occasion, rare even for the only driver that does use this mechanism:
>    sja1105). That was necessary to avoid kfree_skb from freeing the skb.
> 
> 2. Because L2 PTP is a link-local protocol like STP, it requires
>    management routes and deferred xmit with this switch. But as opposed
>    to STP, the deferred work mechanism needs to schedule the packet
>    rather quickly for the TX timstamp to be collected in time and sent
>    to user space. But there is no provision for controlling the
>    scheduling priority of this deferred xmit workqueue. Too bad this is
>    a rather specific requirement for a feature that nobody else uses
>    (more below).
> 
> 3. Perhaps most importantly, it makes the DSA core adhere a bit too
>    much to the NXP company-wide policy "Innovate Where It Doesn't
>    Matter". The sja1105 is probably the only DSA switch that requires
>    some frames sent from the CPU to be routed to the slave port via an
>    out-of-band configuration (register write) rather than in-band (DSA
>    tag). And there are indeed very good reasons to not want to do that:
>    if that out-of-band register is at the other end of a slow bus such
>    as SPI, then you limit that Ethernet flow's throughput to effectively
>    the throughput of the SPI bus. So hardware vendors should definitely
>    not be encouraged to design this way. We do _not_ want more
>    widespread use of this mechanism.
> 
> Luckily we have a solution for each of the 3 issues:
> 
> For 1, we can just remove that variable in the skb->cb and counteract
> the effect of kfree_skb with skb_get, much to the same effect. The
> advantage, of course, being that anybody who doesn't use deferred xmit
> doesn't need to do any extra operation in the hotpath.
> 
> For 2, we can create a kernel thread for each port's deferred xmit work.
> If the user switch ports are named swp0, swp1, swp2, the kernel threads
> will be named swp0_xmit, swp1_xmit, swp2_xmit (there appears to be a 15
> character length limit on kernel thread names). With this, the user can
> change the scheduling priority with chrt $(pidof swp2_xmit).
> 
> For 3, we can actually move the entire implementation to the sja1105
> driver.
> 
> So this patch deletes the generic implementation from the DSA core and
> adds a new one, more adequate to the requirements of PTP TX
> timestamping, in sja1105_main.c.
> 
> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks for addressing this so quickly.
-- 
Florian