[lkml]   [2019]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v3 00/11] Introduces new count-based method for monitoring lockless pagetable walks
John Hubbard <> 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.

> 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

Thank you for helping, John!

Best regards,
Leonardo Bras
[unhandled content-type:application/pgp-signature]
 \ /
  Last update: 2019-09-27 16:47    [W:0.051 / U:36.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site