Re: [PATCH v2][makedumpfile] Fix a data race in multi-threading mode (--num-threads=N)

["HAGIO KAZUHITO(萩尾 一仁)" <k-hagio-ab@nec.com>]

On 2025/07/04 7:35, Tao Liu wrote:
> Hi Petr,
> 
> On Fri, Jul 4, 2025 at 2:31 AM Petr Tesarik <ptesarik@suse.com> wrote:
>>
>> On Tue, 1 Jul 2025 19:59:53 +1200
>> Tao Liu <ltao@redhat.com> wrote:
>>
>>> Hi Kazu,
>>>
>>> Thanks for your comments!
>>>
>>> On Tue, Jul 1, 2025 at 7:38 PM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab@nec.com> wrote:
>>>>
>>>> Hi Tao,
>>>>
>>>> thank you for the patch.
>>>>
>>>> On 2025/06/25 11:23, Tao Liu wrote:
>>>>> A vmcore corrupt issue has been noticed in powerpc arch [1]. It can be
>>>>> reproduced with upstream makedumpfile.
>>>>>
>>>>> When analyzing the corrupt vmcore using crash, the following error
>>>>> message will output:
>>>>>
>>>>>       crash: compressed kdump: uncompress failed: 0
>>>>>       crash: read error: kernel virtual address: c0001e2d2fe48000  type:
>>>>>       "hardirq thread_union"
>>>>>       crash: cannot read hardirq_ctx[930] at c0001e2d2fe48000
>>>>>       crash: compressed kdump: uncompress failed: 0
>>>>>
>>>>> If the vmcore is generated without num-threads option, then no such
>>>>> errors are noticed.
>>>>>
>>>>> With --num-threads=N enabled, there will be N sub-threads created. All
>>>>> sub-threads are producers which responsible for mm page processing, e.g.
>>>>> compression. The main thread is the consumer which responsible for
>>>>> writing the compressed data into file. page_flag_buf->ready is used to
>>>>> sync main and sub-threads. When a sub-thread finishes page processing,
>>>>> it will set ready flag to be FLAG_READY. In the meantime, main thread
>>>>> looply check all threads of the ready flags, and break the loop when
>>>>> find FLAG_READY.
>>>>
>>>> I've tried to reproduce the issue, but I couldn't on x86_64.
>>>
>>> Yes, I cannot reproduce it on x86_64 either, but the issue is very
>>> easily reproduced on ppc64 arch, which is where our QE reported.
>>
>> Yes, this is expected. X86 implements a strongly ordered memory model,
>> so a "store-to-memory" instruction ensures that the new value is
>> immediately observed by other CPUs.
>>
>> FWIW the current code is wrong even on X86, because it does nothing to
>> prevent compiler optimizations. The compiler is then allowed to reorder
>> instructions so that the write to page_flag_buf->ready happens after
>> other writes; with a bit of bad scheduling luck, the consumer thread
>> may see an inconsistent state (e.g. read a stale page_flag_buf->pfn).
>> Note that thanks to how compilers are designed (today), this issue is
>> more or less hypothetical. Nevertheless, the use of atomics fixes it,
>> because they also serve as memory barriers.

Thank you Petr, for the information.  I was wondering whether atomic 
operations might be necessary for the other members of page_flag_buf, 
but it looks like they won't be necessary in this case.

Then I was convinced that the issue would be fixed by removing the 
inconsistency of page_flag_buf->ready.  And the patch tested ok, so ack.

Thanks,
Kazu

> 
> Thanks a lot for your detailed explanation, it's very helpful! I
> haven't thought of the possibility of instruction reordering and
> atomic_rw prevents the reorder.
> 
> Thanks,
> Tao Liu
> 
>>
>> Petr T
>>