| Message ID | 20221224151821.464455-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Fixes for user-only page tracking | expand |
On 24/12/22 16:18, Richard Henderson wrote: > As in page_get_flags, we need to try again with the mmap > lock held if we fail a page lookup. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > index 2c5c10d2e6..c72a806203 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) > int page_check_range(target_ulong start, target_ulong len, int flags) > { > target_ulong last; > + int locked, ret; have_mmap_lock() returns a boolean, can we declare 'locked' as such, ... > > if (len == 0) { > return 0; /* trivial length */ > @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, target_ulong len, int flags) > return -1; /* wrap around */ > } > > + locked = have_mmap_lock(); > while (true) { > PageFlagsNode *p = pageflags_find(start, last); > int missing; > > if (!p) { > - return -1; /* entire region invalid */ > + if (!locked) { > + /* > + * Lockless lookups have false negatives. > + * Retry with the lock held. > + */ > + mmap_lock(); > + locked = -1; ... skip this confusing assignation, ... > + p = pageflags_find(start, last); > + } > + if (!p) { > + ret = -1; /* entire region invalid */ > + break; > + } > } > if (start < p->itree.start) { > - return -1; /* initial bytes invalid */ > + ret = -1; /* initial bytes invalid */ > + break; > } > > missing = flags & ~p->flags; > if (missing & PAGE_READ) { > - return -1; /* page not readable */ > + ret = -1; /* page not readable */ > + break; > } > if (missing & PAGE_WRITE) { > if (!(p->flags & PAGE_WRITE_ORG)) { > - return -1; /* page not writable */ > + ret = -1; /* page not writable */ > + break; > } > /* Asking about writable, but has been protected: undo. */ > if (!page_unprotect(start, 0)) { > - return -1; > + ret = -1; > + break; > } > /* TODO: page_unprotect should take a range, not a single page. */ > if (last - start < TARGET_PAGE_SIZE) { > - return 0; /* ok */ > + ret = 0; /* ok */ > + break; > } > start += TARGET_PAGE_SIZE; > continue; > } > > if (last <= p->itree.last) { > - return 0; /* ok */ > + ret = 0; /* ok */ > + break; > } > start = p->itree.last + 1; > } > + > + if (locked < 0) { ... and check for !locked here? > + mmap_unlock(); > + } > + return ret; > }
On 28/12/22 08:24, Philippe Mathieu-Daudé wrote: > On 24/12/22 16:18, Richard Henderson wrote: >> As in page_get_flags, we need to try again with the mmap >> lock held if we fail a page lookup. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++------- >> 1 file changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c >> index 2c5c10d2e6..c72a806203 100644 >> --- a/accel/tcg/user-exec.c >> +++ b/accel/tcg/user-exec.c >> @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, >> target_ulong end, int flags) >> int page_check_range(target_ulong start, target_ulong len, int flags) >> { >> target_ulong last; >> + int locked, ret; > > have_mmap_lock() returns a boolean, can we declare 'locked' > as such, ... > >> if (len == 0) { >> return 0; /* trivial length */ >> @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, >> target_ulong len, int flags) >> return -1; /* wrap around */ >> } >> + locked = have_mmap_lock(); >> while (true) { >> PageFlagsNode *p = pageflags_find(start, last); >> int missing; >> if (!p) { >> - return -1; /* entire region invalid */ >> + if (!locked) { >> + /* >> + * Lockless lookups have false negatives. >> + * Retry with the lock held. >> + */ >> + mmap_lock(); >> + locked = -1; > > ... skip this confusing assignation, ... > >> + p = pageflags_find(start, last); >> + } >> + if (!p) { >> + ret = -1; /* entire region invalid */ >> + break; >> + } >> } >> if (start < p->itree.start) { >> - return -1; /* initial bytes invalid */ >> + ret = -1; /* initial bytes invalid */ >> + break; >> } >> missing = flags & ~p->flags; >> if (missing & PAGE_READ) { >> - return -1; /* page not readable */ >> + ret = -1; /* page not readable */ >> + break; >> } >> if (missing & PAGE_WRITE) { >> if (!(p->flags & PAGE_WRITE_ORG)) { >> - return -1; /* page not writable */ >> + ret = -1; /* page not writable */ >> + break; >> } >> /* Asking about writable, but has been protected: undo. */ >> if (!page_unprotect(start, 0)) { >> - return -1; >> + ret = -1; >> + break; >> } >> /* TODO: page_unprotect should take a range, not a >> single page. */ >> if (last - start < TARGET_PAGE_SIZE) { >> - return 0; /* ok */ >> + ret = 0; /* ok */ >> + break; >> } >> start += TARGET_PAGE_SIZE; >> continue; >> } >> if (last <= p->itree.last) { >> - return 0; /* ok */ >> + ret = 0; /* ok */ >> + break; >> } >> start = p->itree.last + 1; >> } >> + >> + if (locked < 0) { > > ... and check for !locked here? > >> + mmap_unlock(); >> + } >> + return ret; >> } Modulo using a boolean: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 12/27/22 23:24, Philippe Mathieu-Daudé wrote: > On 24/12/22 16:18, Richard Henderson wrote: >> As in page_get_flags, we need to try again with the mmap >> lock held if we fail a page lookup. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++------- >> 1 file changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c >> index 2c5c10d2e6..c72a806203 100644 >> --- a/accel/tcg/user-exec.c >> +++ b/accel/tcg/user-exec.c >> @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) >> int page_check_range(target_ulong start, target_ulong len, int flags) >> { >> target_ulong last; >> + int locked, ret; > > have_mmap_lock() returns a boolean, can we declare 'locked' > as such, ... > >> if (len == 0) { >> return 0; /* trivial length */ >> @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, target_ulong len, int flags) >> return -1; /* wrap around */ >> } >> + locked = have_mmap_lock(); >> while (true) { >> PageFlagsNode *p = pageflags_find(start, last); >> int missing; >> if (!p) { >> - return -1; /* entire region invalid */ >> + if (!locked) { >> + /* >> + * Lockless lookups have false negatives. >> + * Retry with the lock held. >> + */ >> + mmap_lock(); >> + locked = -1; > > ... skip this confusing assignation, ... > >> + p = pageflags_find(start, last); >> + } >> + if (!p) { >> + ret = -1; /* entire region invalid */ >> + break; >> + } >> } >> if (start < p->itree.start) { >> - return -1; /* initial bytes invalid */ >> + ret = -1; /* initial bytes invalid */ >> + break; >> } >> missing = flags & ~p->flags; >> if (missing & PAGE_READ) { >> - return -1; /* page not readable */ >> + ret = -1; /* page not readable */ >> + break; >> } >> if (missing & PAGE_WRITE) { >> if (!(p->flags & PAGE_WRITE_ORG)) { >> - return -1; /* page not writable */ >> + ret = -1; /* page not writable */ >> + break; >> } >> /* Asking about writable, but has been protected: undo. */ >> if (!page_unprotect(start, 0)) { >> - return -1; >> + ret = -1; >> + break; >> } >> /* TODO: page_unprotect should take a range, not a single page. */ >> if (last - start < TARGET_PAGE_SIZE) { >> - return 0; /* ok */ >> + ret = 0; /* ok */ >> + break; >> } >> start += TARGET_PAGE_SIZE; >> continue; >> } >> if (last <= p->itree.last) { >> - return 0; /* ok */ >> + ret = 0; /* ok */ >> + break; >> } >> start = p->itree.last + 1; >> } >> + >> + if (locked < 0) { > > ... and check for !locked here? No, we may have entered with mmap_locked. Only unlocking if the lock was taken locally. r~
On 28/12/22 18:36, Richard Henderson wrote: > On 12/27/22 23:24, Philippe Mathieu-Daudé wrote: >> On 24/12/22 16:18, Richard Henderson wrote: >>> As in page_get_flags, we need to try again with the mmap >>> lock held if we fail a page lookup. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++------- >>> 1 file changed, 32 insertions(+), 7 deletions(-) >>> >>> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c >>> index 2c5c10d2e6..c72a806203 100644 >>> --- a/accel/tcg/user-exec.c >>> +++ b/accel/tcg/user-exec.c >>> @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, >>> target_ulong end, int flags) >>> int page_check_range(target_ulong start, target_ulong len, int flags) >>> { >>> target_ulong last; >>> + int locked, ret; >> >> have_mmap_lock() returns a boolean, can we declare 'locked' >> as such, ... >> >>> if (len == 0) { >>> return 0; /* trivial length */ >>> @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, >>> target_ulong len, int flags) >>> return -1; /* wrap around */ >>> } >>> + locked = have_mmap_lock(); >>> while (true) { >>> PageFlagsNode *p = pageflags_find(start, last); >>> int missing; >>> if (!p) { >>> - return -1; /* entire region invalid */ >>> + if (!locked) { >>> + /* >>> + * Lockless lookups have false negatives. >>> + * Retry with the lock held. >>> + */ >>> + mmap_lock(); >>> + locked = -1; >> >> ... skip this confusing assignation, ... >> >>> + p = pageflags_find(start, last); >>> + } >>> + if (!p) { >>> + ret = -1; /* entire region invalid */ >>> + break; >>> + } >>> } >>> if (start < p->itree.start) { >>> - return -1; /* initial bytes invalid */ >>> + ret = -1; /* initial bytes invalid */ >>> + break; >>> } >>> missing = flags & ~p->flags; >>> if (missing & PAGE_READ) { >>> - return -1; /* page not readable */ >>> + ret = -1; /* page not readable */ >>> + break; >>> } >>> if (missing & PAGE_WRITE) { >>> if (!(p->flags & PAGE_WRITE_ORG)) { >>> - return -1; /* page not writable */ >>> + ret = -1; /* page not writable */ >>> + break; >>> } >>> /* Asking about writable, but has been protected: undo. */ >>> if (!page_unprotect(start, 0)) { >>> - return -1; >>> + ret = -1; >>> + break; >>> } >>> /* TODO: page_unprotect should take a range, not a >>> single page. */ >>> if (last - start < TARGET_PAGE_SIZE) { >>> - return 0; /* ok */ >>> + ret = 0; /* ok */ >>> + break; >>> } >>> start += TARGET_PAGE_SIZE; >>> continue; >>> } >>> if (last <= p->itree.last) { >>> - return 0; /* ok */ >>> + ret = 0; /* ok */ >>> + break; >>> } >>> start = p->itree.last + 1; >>> } >>> + >>> + if (locked < 0) { >> >> ... and check for !locked here? > > No, we may have entered with mmap_locked. Only unlocking if the lock > was taken locally. Oh, so you are using this variable as tri-state?
On 12/28/22 10:27, Philippe Mathieu-Daudé wrote:
> Oh, so you are using this variable as tri-state?
Yes. Perhaps a comment on the -1?
r~
On 28/12/22 19:30, Richard Henderson wrote: > On 12/28/22 10:27, Philippe Mathieu-Daudé wrote: >> Oh, so you are using this variable as tri-state? > > Yes. Perhaps a comment on the -1? Or name the variable tristate_lock? And a comment :) (No need to respin, you can keep my R-b).
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index 2c5c10d2e6..c72a806203 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -525,6 +525,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags) int page_check_range(target_ulong start, target_ulong len, int flags) { target_ulong last; + int locked, ret; if (len == 0) { return 0; /* trivial length */ @@ -535,42 +536,66 @@ int page_check_range(target_ulong start, target_ulong len, int flags) return -1; /* wrap around */ } + locked = have_mmap_lock(); while (true) { PageFlagsNode *p = pageflags_find(start, last); int missing; if (!p) { - return -1; /* entire region invalid */ + if (!locked) { + /* + * Lockless lookups have false negatives. + * Retry with the lock held. + */ + mmap_lock(); + locked = -1; + p = pageflags_find(start, last); + } + if (!p) { + ret = -1; /* entire region invalid */ + break; + } } if (start < p->itree.start) { - return -1; /* initial bytes invalid */ + ret = -1; /* initial bytes invalid */ + break; } missing = flags & ~p->flags; if (missing & PAGE_READ) { - return -1; /* page not readable */ + ret = -1; /* page not readable */ + break; } if (missing & PAGE_WRITE) { if (!(p->flags & PAGE_WRITE_ORG)) { - return -1; /* page not writable */ + ret = -1; /* page not writable */ + break; } /* Asking about writable, but has been protected: undo. */ if (!page_unprotect(start, 0)) { - return -1; + ret = -1; + break; } /* TODO: page_unprotect should take a range, not a single page. */ if (last - start < TARGET_PAGE_SIZE) { - return 0; /* ok */ + ret = 0; /* ok */ + break; } start += TARGET_PAGE_SIZE; continue; } if (last <= p->itree.last) { - return 0; /* ok */ + ret = 0; /* ok */ + break; } start = p->itree.last + 1; } + + if (locked < 0) { + mmap_unlock(); + } + return ret; } void page_protect(tb_page_addr_t address)
As in page_get_flags, we need to try again with the mmap lock held if we fail a page lookup. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-)