Re: [PATCH for-rc 6/6] IB/hfi1: Remove user expected buffer invalidate race

[Jason Gunthorpe <jgg@nvidia.com>]

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??

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

Jason