| Message ID | 20171007155522.25436-1-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | oslib-posix: Fix compiler warning | expand |
On 10/07/2017 08:55 AM, Stefan Weil wrote: > + char *addr = memset_args->addr; > + uint64_t numpages = memset_args->numpages; > + uint64_t hpagesize = memset_args->hpagesize; > + unsigned i; Match numpages properly while you're at it? r~
Am 09.10.2017 um 23:58 schrieb Richard Henderson: > On 10/07/2017 08:55 AM, Stefan Weil wrote: >> + char *addr = memset_args->addr; >> + uint64_t numpages = memset_args->numpages; >> + uint64_t hpagesize = memset_args->hpagesize; >> + unsigned i; > > Match numpages properly while you're at it? > > > r~ Do you mean using uint64_t for the loop variable i? Sure. I only hesitated to do that because it is so huge. A 64 bit loop variable looks strange, and the loop would take some time if it really uses that range. :-) Feel free to change that if you apply the patch. Or did you think of using size_t for numpages? The current code uses both size_t and uint64_t which could be cleaned as well, maybe even replacing both by uint32_t (or do we expect more pages?). Stefan
On 10/09/2017 10:39 PM, Stefan Weil wrote: > Am 09.10.2017 um 23:58 schrieb Richard Henderson: >> On 10/07/2017 08:55 AM, Stefan Weil wrote: >>> + char *addr = memset_args->addr; >>> + uint64_t numpages = memset_args->numpages; >>> + uint64_t hpagesize = memset_args->hpagesize; >>> + unsigned i; >> >> Match numpages properly while you're at it? >> >> >> r~ > > Do you mean using uint64_t for the loop variable i? Yes. > Sure. I only hesitated to do that because it is so huge. > A 64 bit loop variable looks strange, and the loop would > take some time if it really uses that range. :-) Why would we use a 64-bit variable for numpages if we don't expect such a value? What I see, from spot review only, is an apparent bug -- the iterator type does not match the bound type. r~
Am 10.10.2017 um 16:43 schrieb Richard Henderson: > On 10/09/2017 10:39 PM, Stefan Weil wrote: >> Am 09.10.2017 um 23:58 schrieb Richard Henderson: >>> On 10/07/2017 08:55 AM, Stefan Weil wrote: >>>> + char *addr = memset_args->addr; >>>> + uint64_t numpages = memset_args->numpages; >>>> + uint64_t hpagesize = memset_args->hpagesize; >>>> + unsigned i; >>> Match numpages properly while you're at it? >>> >>> >>> r~ >> Do you mean using uint64_t for the loop variable i? > Yes. > >> Sure. I only hesitated to do that because it is so huge. >> A 64 bit loop variable looks strange, and the loop would >> take some time if it really uses that range. :-) > Why would we use a 64-bit variable for numpages if we don't expect such a > value? What I see, from spot review only, is an apparent bug -- the iterator > type does not match the bound type. If we really expect more than 2^32 pages, touching all those pages will need a significant time even on fast machines. It looks like that function is called in its own thread. Can we be sure that there is no more than one thread of that kind, or do we need more code to disallow a 2nd thread? I don't think that touching the pages twice at the same time makes sense. What about using size_t instead of uint64_t? Parts of the code already use size_t, and it makes a difference for QEMU running on 32 bit hosts. Stefan
On 10/10/2017 09:48 AM, Stefan Weil wrote: > If we really expect more than 2^32 pages, touching all those pages > will need a significant time even on fast machines. Sure. But surely taking a long time is better than silently ignoring high-bits of a quantity. > What about using size_t instead of uint64_t? Parts of the code already > use size_t, and it makes a difference for QEMU running on 32 bit hosts. I'm ok with that, so long as all of the relevant variables are changed, not just that of "i". r~
--- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -301,11 +301,7 @@ static void sigbus_handler(int signal) static void *do_touch_pages(void *arg) { MemsetThread *memset_args = (MemsetThread *)arg; - char *addr = memset_args->addr; - uint64_t numpages = memset_args->numpages; - uint64_t hpagesize = memset_args->hpagesize; sigset_t set, oldset; - int i = 0; /* unblock SIGBUS */ sigemptyset(&set); @@ -315,6 +311,10 @@ static void *do_touch_pages(void *arg) if (sigsetjmp(memset_args->env, 1)) { memset_thread_failed = true; } else { + char *addr = memset_args->addr; + uint64_t numpages = memset_args->numpages; + uint64_t hpagesize = memset_args->hpagesize; + unsigned i; for (i = 0; i < numpages; i++) { /* * Read & write back the same value, so we don't
gcc warning: /qemu/util/oslib-posix.c:304:11: error: variable ‘addr’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] Use also an unsigned loop variable which better matches numpages. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- Please cc qemu-trivial if you think this is trivial enough. Thanks, Stefan index 80086c549f..eb66a6f63c 100644