Re: packet stuck in qdisc : patch proposal

[Vincent Ray <vray@kalrayinc.com>]

----- On May 25, 2022, at 7:43 PM, Vincent Ray vray@kalrayinc.com wrote:

> ----- On May 25, 2022, at 2:40 PM, Guoju Fang gjfang@linux.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@gmail.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.

NB : from what I've understood reading some doc, as this is a fix, it's supposed to go 
in the "net" tree, so I tagged it accordingly in the Subject. Hope 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