Message-ID: <94d68f6301c085fbdd1940cd0f6f7def@walle.cc>
Date:   Mon, 17 May 2021 19:34:18 +0200
From:   Michael Walle <michael@...le.cc>
To:     Yannick Vignon <yannick.vignon@....nxp.com>
Cc:     Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Joakim Zhang <qiangqing.zhang@....com>,
        sebastien.laveze@....nxp.com,
        Yannick Vignon <yannick.vignon@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>,
        Vladimir Oltean <olteanv@...il.com>,
        Vedang Patel <vedang.patel@...el.com>
Subject: Re: [PATCH net v1] net: taprio offload: enforce qdisc to netdev queue
 mapping

Am 2021-05-17 19:06, schrieb Yannick Vignon:
> On 5/15/2021 1:47 AM, Vinicius Costa Gomes wrote:
>> Jakub Kicinski <kuba@...nel.org> writes:
>> 
>>> On Fri, 14 May 2021 13:40:58 -0700 Vinicius Costa Gomes wrote:
>>>> Jakub Kicinski <kuba@...nel.org> writes:
>>>>> You haven't CCed anyone who worked on this Qdisc in the last 2 
>>>>> years :/
>>>>> CCing them now. Comments, anyone?
>>>> 
>>>> I guess I should suggest myself as maintainer, to reduce chances of 
>>>> this
>>>> happening again.
>>> 
>>> Yes, please.
>>> 
>>>>> This looks like a very drastic change. Are you expecting the qdisc 
>>>>> will
>>>>> always be bypassed?
>>>> 
>>>> Only when running in full offload mode it will be bypassed.
>>>> 
>>>> And it's kind of by design, in offload mode, the idea was: configure 
>>>> the
>>>> netdev traffic class to queue mapping, send the schedule to the 
>>>> hardware
>>>> and stay out of the way.
>>>> 
>>>> But as per Yannick's report, it seems that taprio doesn't stay 
>>>> enough
>>>> out of the yay.
>>>> 
>>>>> After a 1 minute looks it seems like taprio is using device queues 
>>>>> in
>>>>> strict priority fashion. Maybe a different model is needed, but a 
>>>>> qdisc
>>>>> with:
>>>>> 
>>>>> enqueue()
>>>>> {
>>>>> 	WARN_ONCE(1)
>>>>> }
>>>>> 
>>>>> really doesn't look right to me.
>>>> 
> 
> My idea was to follow the logic of the other qdiscs dealing with
> hardware multiqueue, namely mq and mqprio. Those do not have any
> enqueue/dequeue callbacks, but instead define an attach callback to
> map the child qdiscs to the HW queues. However, for taprio all those
> callbacks are already defined by the time we choose between software
> and full-offload, so the WARN_ONCE was more out of extra caution in
> case I missed something. If my understanding is correct however, it
> would probably make sense to put a BUG() instead, since those code
> paths should never trigger with this patch.
> 
> OTOH what did bother me a bit is that because I needed an attach
> callback for the full-offload case, I ended up duplicating some code
> from qdisc_graft in the attach callback, so that the software case
> would continue behaving as is.
> 
> Those complexities could be removed by pulling out the full-offload
> case into its own qdisc, but as I said it has other drawbacks.
> 
>>>> This patch takes the "stay out of the way" to the extreme, I kind of
>>>> like it/I am not opposed to it, if I had this idea a couple of years
>>>> ago, perhaps I would have used this same approach.
>>> 
>>> Sorry for my ignorance, but for TXTIME is the hardware capable of
>>> reordering or the user is supposed to know how to send packets?
>> 
>> At least the hardware that I am familiar with doesn't reorder packets.
>> 
>> For TXTIME, we have ETF (the qdisc) that re-order packets. The way
>> things work when taprio and ETF are used together is something like
>> this: taprio only has enough knowledge about TXTIME to drop packets 
>> that
>> would be transmitted outside their "transmission window" (e.g. for
>> traffic class 0 the transmission window is only for 10 to 50, the 
>> TXTIME
>> for a packet is 60, this packet is "invalid" and is dropped). And then
>> when the packet is enqueued to the "child" ETF, it's re-ordered and 
>> then
>> sent to the driver.
>> 
>> And this is something that this patch breaks, the ability of dropping
>> those invalid packets (I really wouldn't like to do this verification
>> inside our drivers). Thanks for noticing this.

Is this really how the taprio should behave? I mean, should the frame
really be dropped by taprio if TXTIME is outside of the window? Why
would taprio bother with TXTIME at all?

> Hmm, indeed, I missed that check (we don't use ETF currently). I'm not
> sure of the best way forward, but here are a few thoughts:
> . The problem only arises for full-offload taprio, not for the
> software or TxTime-assisted cases.
> . I'm not sure mixing taprio(full-offload) with etf(no-offload) is
> very useful, at least with small gate intervals: it's likely you will
> miss your window when trying to send a packet at exactly the right
> time in software (I am usually testing taprio with a 2ms period and a
> 4µs interval for the RT stream).
> . That leaves the case of taprio(full-offload) with etf(offload).
> Right now with the current stmmac driver config, a packet whose tstamp
> is outside its gate interval will be sent on the next interval (and
> block the queue).

If both are offloaded, are the h/w queues reordered if there is a
new frame with an earlier TXTIME? Or will the queue be blocked by a
frame with a later transmission time?

TBH I've never understood why the ETF qdisc will drop frames with
TXTIME in the past. I mean it makes sense with hardware offloading. But
without it, how can you make sure the kernel will wake up the queue
at just the right time to be able to send it. You can juggle the delta
parameter but on you don't want to send to too early, but on the other
hand the frame will likely be dropped if delta is too small. What am
I misssing here?

> . The stmmac hardware supports an expiryTime, currently unsupported in
> the stmmac driver, which I think could be used to drop packets whose
> tstamps are wrong (the packet would be dropped once the tstamp
> "expires"). We'd need to add an API for configuration though, and it
> should be noted that the stmmac config for this is global to the MAC,
> not per-queue (so a config through sch-etf would affect all queues).
> . In general using taprio(full-offload) with etf(offload) will incur a
> small latency penalty: you need to post the packet before the ETF
> qdisc wakes up (plus some margin), and the ETF qdisc must wake up
> before the tx stamp (plus some margin). If possible (number of
> streams/apps < number of hw queues), it would be better to just use
> taprio(full-offload) alone, since the app will need to post the packet
> before the gate opens (so plus one margin, not 2).
> 
> 
>>> 
>>> My biggest problem with this patch is that unless the application is
>>> very careful that WARN_ON_ONCE(1) will trigger. E.g. if softirq is
>>> servicing the queue when the application sends - the qdisc will not
>>> be bypassed, right?
> 
> See above, unless I'm mistaken the "root" qdisc is never
> enqueued/dequeued for multi-queue aware qdiscs.
> 
>>>> I am now thinking if this idea locks us out of anything.
>>>> 
>>>> Anyway, a nicer alternative would exist if we had a way to tell the 
>>>> core
>>>> "this qdisc should be bypassed" (i.e. don't call 
>>>> enqueue()/dequeue())
>>>> after init() runs.
>>> 
> 
> Again, I don't think enqueue/dequeue are called unless the HW queues
> point to the root qdisc. But this does raise an interesting point: the
> "scheduling" issue I observed was on the dequeue side, when all the
> queues were dequeued within the RT process context. If we could point
> the enqueue side to the taprio qdisc and the dequeue side to the child
> qdiscs, that would probably work (but I fear that would be a
> significant change in the way the qdisc code works).
> 
>>> I don't think calling enqueue() and dequeue() is a problem. The 
>>> problem
>>> is that RT process does unrelated work.
>> 
>> That is true. But this seems like a much bigger (or at least more
>> "core") issue.
>> 
>> 
>> Cheers,
>> 

-- 
-michael

Powered by blists - more mailing lists