On 9/4/24 18:16, Jens Axboe wrote:
> On 9/4/24 10:08 AM, Bernd Schubert wrote:
>> Hi Jens,
>>
>> thanks for your help.
>>
>> On 9/4/24 17:47, Jens Axboe wrote:
>>> On 9/1/24 7:37 AM, Bernd Schubert wrote:
>>>> This is to allow copying into the buffer from the application
>>>> without the need to copy in ring context (and with that,
>>>> the need that the ring task is active in kernel space).
>>>>
>>>> Also absolutely needed for now to avoid this teardown issue
>>>
>>> I'm fine using these helpers, but they are absolutely not needed to
>>> avoid that teardown issue - well they may help because it's already
>>> mapped, but it's really the fault of your handler from attempting to map
>>> in user pages from when it's teardown/fallback task_work. If invoked and
>>> the ring is dying or not in the right task (as per the patch from
>>> Pavel), then just cleanup and return -ECANCELED.
>>
>> As I had posted on Friday/Saturday, it didn't work. I had added a
>> debug pr_info into Pavels patch, somehow it didn't trigger on PF_EXITING
>> and I didn't further debug it yet as I was working on the pin anyway.
>> And since Monday occupied with other work...
>
> Then there's something wrong with that patch, as it definitely should
> work. How did you reproduce the teardown crash? I'll take a look here.
Thank you! In this specific case
1) Run passthrough_hp with --debug-fuse
2) dd if=/dev/zero of=/scratch/test/testfile bs=1M count=1
Then on the console that has passthrough_hp output and runs slow with my
ASAN/etc kernel: ctrl-z and kill -9 %
I guess a pkill -9 passthrough_hp should also work
But I can investigate later on myself what is the issue with PF_EXITING,
just not today and maybe not tomorrow either.
>
> That said, it may indeed be the better approach to pin upfront. I just
> want to make sure it's not done as a bug fix for something that should
> not be happening.
>
>> For this series it is needed to avoid kernel crashes. If we can can fix
>> patch 15 and 16, the better. Although we will still later on need it as
>> optimization.
>
> Yeah exactly, didn't see this before typing the above :-)
>
>>>> +/*
>>>> + * Copy from memmap.c, should be exported
>>>> + */
>>>> +static void io_pages_free(struct page ***pages, int npages)
>>>> +{
>>>> + struct page **page_array = *pages;
>>>> +
>>>> + if (!page_array)
>>>> + return;
>>>> +
>>>> + unpin_user_pages(page_array, npages);
>>>> + kvfree(page_array);
>>>> + *pages = NULL;
>>>> +}
>>>
>>> I noticed this and the mapping helper being copied before seeing the
>>> comments - just export them from memmap.c and use those rather than
>>> copying in the code. Add that as a prep patch.
>>
>> No issue to do that either. The hard part is then to get it through
>> different branches. I had removed the big optimization of
>> __wake_up_on_current_cpu in this series, because it needs another
>> export.
>
> It's not that hard, just split it out in the next patch and I'll be
> happy to ack/review it so it can go in with the other patches rather
> than needing to go in separately.
Great thank you very much, will do!
Thanks,
Bernd
