Nouveau - Nov 2019 - [PATCH v2 02/15] mm/mmu_notifier: add an interval tree notifier

On Thu, Nov 07, 2019 at 04:04:08PM -0500, Jerome Glisse
wrote:
> On Thu, Nov 07, 2019 at 08:11:06PM +0000, Jason Gunthorpe wrote:
> > On Wed, Nov 06, 2019 at 09:08:07PM -0500, Jerome Glisse wrote:
> > 
> > > > 
> > > > Extra credit: IMHO, this clearly deserves to all be in a new
mmu_range_notifier.h
> > > > header file, but I know that's extra work. Maybe later
as a follow-up patch,
> > > > if anyone has the time.
> > > 
> > > The range notifier should get the event too, it would be a waste,
i think it is
> > > an oversight here. The release event is fine so NAK to you
separate event. Event
> > > is really an helper for notifier i had a set of patch for nouveau
to leverage
> > > this i need to resucite them. So no need to split thing, i would
just forward
> > > the event ie add event to mmu_range_notifier_ops.invalidate() i
failed to catch
> > > that in v1 sorry.
> > 
> > I think what you mean is already done?
> > 
> > struct mmu_range_notifier_ops {
> > 	bool (*invalidate)(struct mmu_range_notifier *mrn,
> > 			   const struct mmu_notifier_range *range,
> > 			   unsigned long cur_seq);
> 
> Yes it is sorry, i got confuse with mmu_range_notifier and
mmu_notifier_range :)
> It is almost a palyndrome structure ;)
Lets change the name then, this is clearly not working. I'll reflow
everything tomorrow

Jason

On Fri, Nov 08, 2019 at 12:32:25AM +0000, Jason Gunthorpe
wrote:
> On Thu, Nov 07, 2019 at 04:04:08PM -0500, Jerome Glisse wrote:
> > On Thu, Nov 07, 2019 at 08:11:06PM +0000, Jason Gunthorpe wrote:
> > > On Wed, Nov 06, 2019 at 09:08:07PM -0500, Jerome Glisse wrote:
> > > 
> > > > > 
> > > > > Extra credit: IMHO, this clearly deserves to all be in
a new mmu_range_notifier.h
> > > > > header file, but I know that's extra work. Maybe
later as a follow-up patch,
> > > > > if anyone has the time.
> > > > 
> > > > The range notifier should get the event too, it would be a
waste, i think it is
> > > > an oversight here. The release event is fine so NAK to you
separate event. Event
> > > > is really an helper for notifier i had a set of patch for
nouveau to leverage
> > > > this i need to resucite them. So no need to split thing, i
would just forward
> > > > the event ie add event to
mmu_range_notifier_ops.invalidate() i failed to catch
> > > > that in v1 sorry.
> > > 
> > > I think what you mean is already done?
> > > 
> > > struct mmu_range_notifier_ops {
> > > 	bool (*invalidate)(struct mmu_range_notifier *mrn,
> > > 			   const struct mmu_notifier_range *range,
> > > 			   unsigned long cur_seq);
> > 
> > Yes it is sorry, i got confuse with mmu_range_notifier and
mmu_notifier_range :)
> > It is almost a palyndrome structure ;)
> 
> Lets change the name then, this is clearly not working. I'll reflow
> everything tomorrow
Semantic patch to do that run from your linux kernel directory with your patch
applied (you can run it one patch after the other and the git commit -a --fixup
HEAD)

spatch --sp-file name-of-the-file-below --dir . --all-includes --in-place

%< ------------------------------------------------------------------
@@
@@
struct
-mmu_range_notifier
+mmu_interval_notifier

@@
@@
struct
-mmu_range_notifier
+mmu_interval_notifier
{...};

// Change mrn name to mmu_in
@@
struct mmu_interval_notifier *mrn;
@@
-mrn
+mmu_in

@@
identifier fn;
@@
fn(..., 
-struct mmu_interval_notifier *mrn,
+struct mmu_interval_notifier *mmu_in,
...) {...}
------------------------------------------------------------------ >%

You need coccinelle (which provides spatch). It is untested but it should work
also i could not come up with a nice name to update mrn as min is way too
confusing. If you have better name feel free to use it.

Oh and coccinelle is pretty clever about code formating so it should do a good
jobs at keeping things nicely formated and align.

Cheers,
J?r?me

On Thu, Nov 07, 2019 at 09:00:34PM -0500, Jerome Glisse
wrote:
> On Fri, Nov 08, 2019 at 12:32:25AM +0000, Jason Gunthorpe wrote:
> > On Thu, Nov 07, 2019 at 04:04:08PM -0500, Jerome Glisse wrote:
> > > On Thu, Nov 07, 2019 at 08:11:06PM +0000, Jason Gunthorpe wrote:
> > > > On Wed, Nov 06, 2019 at 09:08:07PM -0500, Jerome Glisse
wrote:
> > > > 
> > > > > > 
> > > > > > Extra credit: IMHO, this clearly deserves to all
be in a new mmu_range_notifier.h
> > > > > > header file, but I know that's extra work.
Maybe later as a follow-up patch,
> > > > > > if anyone has the time.
> > > > > 
> > > > > The range notifier should get the event too, it would
be a waste, i think it is
> > > > > an oversight here. The release event is fine so NAK to
you separate event. Event
> > > > > is really an helper for notifier i had a set of patch
for nouveau to leverage
> > > > > this i need to resucite them. So no need to split
thing, i would just forward
> > > > > the event ie add event to
mmu_range_notifier_ops.invalidate() i failed to catch
> > > > > that in v1 sorry.
> > > > 
> > > > I think what you mean is already done?
> > > > 
> > > > struct mmu_range_notifier_ops {
> > > > 	bool (*invalidate)(struct mmu_range_notifier *mrn,
> > > > 			   const struct mmu_notifier_range *range,
> > > > 			   unsigned long cur_seq);
> > > 
> > > Yes it is sorry, i got confuse with mmu_range_notifier and
mmu_notifier_range :)
> > > It is almost a palyndrome structure ;)
> > 
> > Lets change the name then, this is clearly not working. I'll
reflow
> > everything tomorrow
> 
> Semantic patch to do that run from your linux kernel directory with your
patch
> applied (you can run it one patch after the other and the git commit -a
--fixup HEAD)
> 
> spatch --sp-file name-of-the-file-below --dir . --all-includes --in-place
> 
> %< ------------------------------------------------------------------
> @@
> @@
> struct
> -mmu_range_notifier
> +mmu_interval_notifier
> 
> @@
> @@
> struct
> -mmu_range_notifier
> +mmu_interval_notifier
> {...};
> 
> // Change mrn name to mmu_in
> @@
> struct mmu_interval_notifier *mrn;
> @@
> -mrn
> +mmu_in
> 
> @@
> identifier fn;
> @@
> fn(..., 
> -struct mmu_interval_notifier *mrn,
> +struct mmu_interval_notifier *mmu_in,
> ...) {...}
> 
> You need coccinelle (which provides spatch). It is untested but it should
work
> also i could not come up with a nice name to update mrn as min is way too
> confusing. If you have better name feel free to use it.
I used 'mni' as we already use 'mn' to refer to the notifier,
and
'mmu_in' looks like some input parameter or something

It mostly worked, lots of comments to fix manually though:

https://github.com/jgunthorpe/linux/commits/mmu_notifier

Thanks,
Jason