Re: [PATCH v3 00/11] Introduces new count-based method for monitoring lockless pagetable walks

[Leonardo Bras]

John Hubbard <jhubbard@xxxxxxxxxx> writes:

> Hi Leonardo,
>
> Thanks for adding linux-mm to CC for this next round of reviews. For the benefit
> of any new reviewers, I'd like to add that there are some issues that were discovered
> while reviewing the v2 patchset, that are not (yet) addressed in this v3 series.

> Since those issues are not listed in the cover letter above, I'll list them here

Thanks for bringing that.
The cover letter is a great place to put this info, I will keep that in
mind for future patchsets.

>
> 1. The locking model requires a combination of disabling interrupts and
> atomic counting and memory barriers, but
>
> 	a) some memory barriers are missing
> 	(start/end_lockless_pgtbl_walk), and

It seems that it works fine today because of the amount of intructions
executed between the irq_disable / start_lockless_pgtbl_walk and where
the THP collapse/split can happen. (It's very unlikely that it reorders
that much).

But I don't think it would be so bad to put a memory barrier after
irq_disable just in case.

> 	b) some cases (patch #8) fail to disable interrupts

I have done some looking into that, and it seems that some uses of
{start,end}_lockless_pgtbl_walk are unneeded, because they operate in
(nested) guest pgd and I was told it's safe against THP split/collapse.

In other uses, there is no interrupt disable because the function is
called in real mode, with MSR_EE=0, and there we have instructions
disabled, so there is no need to disable them again.

>
> ...so the synchronization appears to be inadequate. (And if it *is* adequate, then
> definitely we need the next item, to explain it.)


>
> 2. Documentation of the synchronization/locking model needs to exist, once we
> figure out the exact details of (1).

I will add the missing doc in the code, so it may be easier to
understand in the future.

>
> 3. Related to (1), I've asked to change things so that interrupt controls and 
> atomic inc/dec are in the same start/end calls--assuming, of course, that the
> caller can tolerate that. 

I am not sure if it would be ok to use irq_{save,restore} in real mode,
I will do some more reading of the docs before addressing this. 
>
> 4. Please see the v2 series for any other details I've missed.
>
> thanks,
> -- 
> John Hubbard
> NVIDIA
>

Thank you for helping, John!

Best regards,
Leonardo Bras

Attachment: signature.asc
Description: This is a digitally signed message part