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.
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.
--
Jens Axboe
