netdev - Re: [PATCH v2 net-next 2/3] net: dsa: Make deferred

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 4 Jan 2020 20:39:16 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, vivien.didelot@...il.com,
        andrew@...n.ch, davem@...emloft.net
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 2/3] net: dsa: Make deferred_xmit private to
 sja1105



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@...il.com>
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>

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

Thanks for addressing this so quickly.
-- 
Florian