On 1/19/2023 12:05 PM, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2023 at 10:00:39AM -0600, Dean Luick wrote:
>> On 1/18/2023 6:42 AM, Jason Gunthorpe wrote:
>>> On Tue, Jan 17, 2023 at 01:19:14PM -0600, Dean Luick wrote:
>>>> On 1/16/2023 9:41 AM, Jason Gunthorpe wrote:
>>>>> On Mon, Jan 09, 2023 at 12:31:31PM -0500, Dennis Dalessandro wrote:
>>>>>
>>>>>> + if (fd->use_mn) {
>>>>>> + ret = mmu_interval_notifier_insert(
>>>>>> + &tidbuf->notifier, current->mm,
>>>>>> + tidbuf->vaddr, tidbuf->npages * PAGE_SIZE,
>>>>>> + &tid_cover_ops);
>>>>>
>>>>> This is still not the right way to use these notifiers, you should be
>>>>> removing the calls to mmu_notifier_register()
>>>>
>>>> I am confused by your comment. This is the user expected receive
>>>> code. There are no calls to mmu_notifier_register() here. You
>>>> removed those calls when you added the FIXME. The Send DMA side
>>>> still has calls to mmu_notifier_register(). This series is all
>>>> about user expected receive.
>>>
>>> Then something else seems wrong because you shouldn't be removing the
>>> notifiers in the same function you add them
>>
>> The add-then-remove is intentional. The purpose is to make sure
>> there are no invalidates while we pin pages and set up the
>> "permanent" notifiers that cover exact ranges based on sequential
>> pages and how the DMA hardware is programmed. Once the programmed
>> hardware range notifiers are in place, the covering range serves no
>> purpose and can be removed.
>
> Uh, that doesn't sound very good, it is very expensive to create these
> notifiers, they were not intended to be overly narrowed like this -
> you are better to have a wider range and deal with the rare false hit
> than to take the cost of register/unregister on every activity??
The temporary cover is a compromise effort to avoid the following situation. Suppose, as you suggest, we keep a single range on the registration call. Then:
User registers range A. The hardware breaks it up into several separate pieces - "TIDs". Due to pressure, software releases several, but not all the pieces of A. Because not all of A is gone, the notify range is retained.
Later, software registers range B, which includes parts of the original A. A new single range is registered. Since this overlaps with A, we now have multiple range notifiers over the same memory range. All would still be functionally correct, since there would only be one owner of each TID. But we now have overlapping permanent notify ranges.
Repeat this several times and you can have a mess of overlapping ranges, each with a little piece that is actually live.
With a unique notifier over each TID, the notifier can be dismissed when the TID is released.
We judged it would be simpler and better to keep the smaller, more specific ranges that matched the programmed hardware. No searching or checking needed. A 1-1 correspondence.
None of this would be needed if there was a correct, safe way to hold off all memory invalidates in a target range (or just in general) while we set up our per-TID notifiers.
> Also I'm not entirely sure this can be race free, having two notifiers
> responsible for the same memory at the same time sounds like Danger
> Danger sort of situation..
No danger at all. The temporary "cover" range only registers that something has happened in the whole range. The "permanent", aka TID, range is responsible for clearing the hardware. Separate responsibilities.
-Dean
External recipient
