Date:   Wed, 25 May 2022 19:43:49 +0200 (CEST)
From:   Vincent Ray <vray@...rayinc.com>
To:     Guoju Fang <gjfang@...ux.alibaba.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        linyunsheng <linyunsheng@...wei.com>
Cc:     davem <davem@...emloft.net>,
        方国炬 <guoju.fgj@...baba-inc.com>,
        kuba <kuba@...nel.org>, netdev <netdev@...r.kernel.org>,
        Samuel Jones <sjones@...rayinc.com>,
        vladimir oltean <vladimir.oltean@....com>,
        Remy Gauguey <rgauguey@...rayinc.com>, will <will@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>, pabeni@...hat.com
Subject: Re: packet stuck in qdisc : patch proposal



----- On May 25, 2022, at 2:40 PM, Guoju Fang gjfang@...ux.alibaba.com wrote:

> On 2022/5/25 18:45, Yunsheng Lin wrote:
>> On 2022/5/25 17:44, Vincent Ray wrote:
>>> ----- On May 24, 2022, at 10:17 PM, Eric Dumazet eric.dumazet@...il.com wrote:
>>>
>>>> On 5/24/22 10:00, Vincent Ray wrote:
>>>>> All,
>>>>>
>>>>> I confirm Eric's patch works well too, and it's better and clearer than mine.
>>>>> So I think we should go for it, and the one from Guoju in addition.
>>>>>
>>>>> @Eric : I see you are one of the networking maintainers, so I have a few
>>>>> questions for you :
>>>>>
>>>>> a) are you going to take care of these patches directly yourself, or is there
>>>>> something Guoju or I should do to promote them ?
>>>>
>>>> I think this is totally fine you take ownership of the patch, please
>>>> send a formal V2.
>>>>
>>>> Please double check what patchwork had to say about your V1 :
>>>>
>>>>
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/1684598287.15044793.1653314052575.JavaMail.zimbra@kalray.eu/
>>>>
>>>>
>>>> And make sure to address the relevant points
>>>
>>> OK I will.
>>> If you agree, I will take your version of the fix (test_and_set_bit()), keeping
>>> the commit message
>>> similar to my original one.
>>>
>>> What about Guoju's patch ?
>> 
>> @Guoju, please speak up if you want to handle the patch yourself.
> 
> Hi Yunsheng, all,
> 
> I rewrite the comments of my patch and it looks a little clearer. :)
> 
> Thank you.
> 
> Best regards,

Guoju : shouldn't you also include the same Fixes tag suggested by YunSheng ?

Here's mine, attached. Hope it's well formatted this time. Tell me.
I don't feel quite confident with the submission process to produce the series myself, so I'll let Eric handle it if it's ok.

> 
>> 
>>> (adding a smp_mb() between the spin_unlock() and test_bit() in qdisc_run_end()).
>>> I think it is also necessary though potentially less critical.
>>> Do we embed it in the same patch ? or patch series ?
>> 
>> Guoju's patch fixes the commit a90c57f2cedd, so "patch series"
>> seems better if Guoju is not speaking up to handle the patch himself.
>> 
>> 
>>>
>>> @Guoju : have you submitted it for integration ?
>>>
>>>
>>>> The most important one is the lack of 'Signed-off-by:' tag, of course.
>>>>
>>>>
>>>>> b) Can we expect to see them land in the mainline soon ?
>>>>
>>>> If your v2 submission is correct, it can be merged this week ;)
>>>>
>>>>
>>>>>
>>>>> c) Will they be backported to previous versions of the kernel ? Which ones ?
>>>>
>>>> You simply can include a proper Fixes: tag, so that stable teams can
>>>> backport
>>>>
>>>> the patch to all affected kernel versions.
>>>>
>>>
>>> Here things get a little complicated in my head ;-)
>>> As explained, I think this mechanism has been bugged, in a way or an other, for
>>> some time, perhaps since the introduction
>>> of lockless qdiscs (4.16) or somewhere between 4.16 and 5.14.
>>> It's hard to tell at a glance since the code looks quite different back then.
>>> Because of these changes, a unique patch will also only apply up to a certain
>>> point in the past.
>>>
>>> However, I think the bug became really critical only with the introduction of
>>> "true bypass" behavior
>>> in lockless qdiscs by YunSheng in 5.14, though there may be scenarios where it
>>> is a big deal
>>> even in no-bypass mode.
>> 
>> 
>> commit 89837eb4b246 tried to fix that, but it did not fix it completely, and
>> that commit should has
>> been back-ported to the affected kernel versions as much as possible, so I think
>> the Fixes tag
>> should be:
>> 
>> Fixes: 89837eb4b246 ("net: sched: add barrier to ensure correct ordering for
>> lockless qdisc")
>> 
>>>
>>> => I suggest we only tag it for backward fix up to the 5.14, where it should
>>> apply smoothly,
>>>   and we live with the bug for versions before that.
>>> This would mean that 5.15 LT can be patched but no earlier LT
>>>   
>>> What do you think ?
>>>
>>> BTW : forgive my ignorance, but are there any kind of "Errata Sheet" or similar
>>> for known bugs that
>>> won't be fixed in a given kernel ?
>>>
>>>>
>>>>
>>>>>
>>>>> Thanks a lot, best,
>>>>
>>>> Thanks a lot for working on this long standing issue.
>>>>
>>>>
>>>>
>>>>
>>>> To declare a filtering error, please use the following link :
>>>> https://www.security-mail.net/reporter.php?mid=7009.628d3d4c.37c04.0&r=vray%40kalrayinc.com&s=eric.dumazet%40gmail.com&o=Re%3A+packet+stuck+in+qdisc+%3A+patch+proposal&verdict=C&c=0ca08e7b7e420d1ab014cda67db48db71df41f5f
>>>
>>>
>>>
>>>
>>> .
>>>
> 
> To declare a filtering error, please use the following link :
> https://www.security-mail.net/reporter.php?mid=2c69.628e23bf.45908.0&r=vray%40kalrayinc.com&s=gjfang%40linux.alibaba.com&o=Re%3A+packet+stuck+in+qdisc+%3A+patch+proposal&verdict=C&c=6106070134039ab6725b6d3de67bd24d624c8b51



Download attachment "0001-net-sched-fixed-barrier-to-prevent-skbuff-sticking-i.patch" of type "application/mbox" (5550 bytes)

Powered by blists - more mailing lists