Re: [PATCH net-next 0/2] Improvements to the DSA deferred xmit

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

On 1/2/20 2:47 PM, Vladimir Oltean wrote:
> Hi David,
> 
> On Thu, 2 Jan 2020 at 23:49, David Miller <davem@davemloft.net> wrote:
>>
>> Two comments about this patch series, I think it needs more work:
>>
> 
> Thanks for looking at this series.
> 
>> 1) This adds the thread and the xmit queue but not code that actually
>>    uses it.  You really have to provide the support code in the driver
>>    at the same time you add the new facitlity so we can actually see
>>    how it'll be used.
>>
> 
> There is no API change here. There was, and still is, a single caller
> of dsa_defer_xmit in the kernel, from net/dsa/tag_sja1105.c:
> 
>     if (unlikely(sja1105_is_link_local(skb)))
>         return dsa_defer_xmit(skb, netdev);
> 
> The whole difference is that what used to be a schedule_work() in that
> function is now a kthread_queue_work() call.
> 
>> 2) Patch #1 talks about a tradeoff.  Replacing the CB initialization of
>>    the field skb_get().  But this skb_get() is an atomic operation and
>>    thus much more expensive for users of the deferred xmit scheme.
>>
> 
> Ok, I'll admit I hadn't considered the exact penalty introduced by
> skb_get, but I think it is a matter of proportions.
> Worst case I expect no more than 64 packets per second to be
> transmitted using the deferred xmit mechanism: there are 4 switch
> ports, PTP runs with a sync interval of 1/8 seconds, and the STP hello
> timer is 2 seconds. So, not a lot of traffic.
> On the other hand, clearing the deferred_xmit bool from the skb->cb is
> something that everybody else (including this driver for "normal"
> traffic) needs to do at line rate, just for the above 64 packets per
> second (in the worst case) to be possible.

I sincerely think your transmit path is so radically different that your
sja1105 driver should simply be absorbing all of this logic and the core
DSA framework should be free of any deferred transmit logic. Can you
consider doing that before the merge window ends?
-- 
Florian