| Message ID | 478545ddab5b50a804bed4eea1c8f38e155335fa.1435723168.git.serge.vakulenko@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 2015-06-30 21:12, Serge Vakulenko wrote: > Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> > --- > hw/mips/cputimer.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c > index 4f02a9f..94a29df 100644 > --- a/hw/mips/cputimer.c > +++ b/hw/mips/cputimer.c > @@ -25,21 +25,13 @@ > #include "qemu/timer.h" > #include "sysemu/kvm.h" > > -#define TIMER_FREQ 100 * 1000 * 1000 > - > -/* XXX: do not use a global */ > +/* Generate a random TLB index. > + * Skip wired entries. */ > uint32_t cpu_mips_get_random (CPUMIPSState *env) > { > - static uint32_t lfsr = 1; > - static uint32_t prev_idx = 0; > - uint32_t idx; > - /* Don't return same value twice, so get another value */ > - do { > - lfsr = (lfsr >> 1) ^ (-(lfsr & 1u) & 0xd0000001u); > - idx = lfsr % (env->tlb->nb_tlb - env->CP0_Wired) + env->CP0_Wired; > - } while (idx == prev_idx); > - prev_idx = idx; > - return idx; > + env->CP0_Random = env->CP0_Wired + > + random() % (env->tlb->nb_tlb - env->CP0_Wired); > + return env->CP0_Random; > } > > /* MIPS R4K timer */ Can you please give us more details about what issue you are trying to fix there? Especially I don't understand about the "skip wired entries" part. It seems the original code handles the wired entries correctly, and at least your patch doesn't seem to change anything regarded that part. Secondly, I don't think calling random() is the correct thing to do. It's an expensive function that is not thread safe. Quoting the specification: "Within the required constraints of the upper and lower bounds, the manner in which the processor selects values for the Random register is implementation-dependent." So it's fine if we use a PRNG like the current code, but I agree we might want to improve it if it has some issues. We want to keep its value reproducible though so that the icount mode works as expected.
On Tue, 30 Jun 2015 21:12:31 -0700 Serge Vakulenko <serge.vakulenko@gmail.com> wrote: > Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> > --- > hw/mips/cputimer.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c > index 4f02a9f..94a29df 100644 > --- a/hw/mips/cputimer.c > +++ b/hw/mips/cputimer.c > @@ -25,21 +25,13 @@ > #include "qemu/timer.h" > #include "sysemu/kvm.h" > > -#define TIMER_FREQ 100 * 1000 * 1000 > - This is a part of the 'Speed of MIPS CPU timer made configurable per platform.' patch. -- Best regards, Antony Pavlov
On Wed, 1 Jul 2015, Aurelien Jarno wrote: > Secondly, I don't think calling random() is the correct thing to do. > It's an expensive function that is not thread safe. Quoting the > specification: > > "Within the required constraints of the upper and lower bounds, the > manner in which the processor selects values for the Random register > is implementation-dependent." > > So it's fine if we use a PRNG like the current code, but I agree we > might want to improve it if it has some issues. We want to keep its > value reproducible though so that the icount mode works as expected. Implementations often implement CP0.Random as a free-running counter that decrements between the bounds set as each instruction executes. Maciej
On Wed, Jul 1, 2015 at 3:11 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2015-06-30 21:12, Serge Vakulenko wrote: >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> >> --- >> hw/mips/cputimer.c | 18 +++++------------- >> 1 file changed, 5 insertions(+), 13 deletions(-) >> >> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c >> index 4f02a9f..94a29df 100644 >> --- a/hw/mips/cputimer.c >> +++ b/hw/mips/cputimer.c >> @@ -25,21 +25,13 @@ >> #include "qemu/timer.h" >> #include "sysemu/kvm.h" >> >> -#define TIMER_FREQ 100 * 1000 * 1000 >> - >> -/* XXX: do not use a global */ >> +/* Generate a random TLB index. >> + * Skip wired entries. */ >> uint32_t cpu_mips_get_random (CPUMIPSState *env) >> { >> - static uint32_t lfsr = 1; >> - static uint32_t prev_idx = 0; >> - uint32_t idx; >> - /* Don't return same value twice, so get another value */ >> - do { >> - lfsr = (lfsr >> 1) ^ (-(lfsr & 1u) & 0xd0000001u); >> - idx = lfsr % (env->tlb->nb_tlb - env->CP0_Wired) + env->CP0_Wired; >> - } while (idx == prev_idx); >> - prev_idx = idx; >> - return idx; >> + env->CP0_Random = env->CP0_Wired + >> + random() % (env->tlb->nb_tlb - env->CP0_Wired); >> + return env->CP0_Random; >> } >> >> /* MIPS R4K timer */ > > Can you please give us more details about what issue you are trying to > fix there? Especially I don't understand about the "skip wired entries" > part. It seems the original code handles the wired entries correctly, > and at least your patch doesn't seem to change anything regarded that > part. The original code looks fine by itself. But when you try to run in for nb_tlb=16 and CP0_Wired=1, you get a sequence: 15, 6, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2... This is what happens when 4.4bsd kernel starts on pic32mz processor. It makes the VM subsystem a bit crazy. Later the sequence becomes better, but I think it makes sense to improve it somehow. > Secondly, I don't think calling random() is the correct thing to do. > It's an expensive function that is not thread safe. Quoting the > specification: > > "Within the required constraints of the upper and lower bounds, the > manner in which the processor selects values for the Random register > is implementation-dependent." > > So it's fine if we use a PRNG like the current code, but I agree we > might want to improve it if it has some issues. We want to keep its > value reproducible though so that the icount mode works as expected. I agree that random() is somewhat heavy routine, and we don't need so much randomness here. OK, in the next version of this patch set I'll propose another variant of simple random generator, like TLCG or something. > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net
On Thu, Jul 2, 2015 at 12:52 AM, Antony Pavlov <antonynpavlov@gmail.com> wrote: > On Tue, 30 Jun 2015 21:12:31 -0700 > Serge Vakulenko <serge.vakulenko@gmail.com> wrote: > >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> >> --- >> hw/mips/cputimer.c | 18 +++++------------- >> 1 file changed, 5 insertions(+), 13 deletions(-) >> >> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c >> index 4f02a9f..94a29df 100644 >> --- a/hw/mips/cputimer.c >> +++ b/hw/mips/cputimer.c >> @@ -25,21 +25,13 @@ >> #include "qemu/timer.h" >> #include "sysemu/kvm.h" >> >> -#define TIMER_FREQ 100 * 1000 * 1000 >> - > > This is a part of the 'Speed of MIPS CPU timer made configurable per platform.' patch. Oops... Thanks for pointing this out. I'll move it to a proper place in the next version of the patch set. Regards, --Serge > > -- > Best regards, > Antony Pavlov
On Fri, Jul 3, 2015 at 2:39 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote: > On Wed, 1 Jul 2015, Aurelien Jarno wrote: > >> Secondly, I don't think calling random() is the correct thing to do. >> It's an expensive function that is not thread safe. Quoting the >> specification: >> >> "Within the required constraints of the upper and lower bounds, the >> manner in which the processor selects values for the Random register >> is implementation-dependent." >> >> So it's fine if we use a PRNG like the current code, but I agree we >> might want to improve it if it has some issues. We want to keep its >> value reproducible though so that the icount mode works as expected. > > Implementations often implement CP0.Random as a free-running counter that > decrements between the bounds set as each instruction executes. That's true as a first approximation, but in a real core the picture is usually a bit more complicated. Decrementing every clock cycle consumes too much energy. Decrementing only on TLBWR instruction makes the sequence too predictable and can result in extra thrashing for some applications. Regards, --Serge > Maciej
On 2015-07-05 17:03, Serge Vakulenko wrote: > On Wed, Jul 1, 2015 at 3:11 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2015-06-30 21:12, Serge Vakulenko wrote: > >> Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> > >> --- > >> hw/mips/cputimer.c | 18 +++++------------- > >> 1 file changed, 5 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c > >> index 4f02a9f..94a29df 100644 > >> --- a/hw/mips/cputimer.c > >> +++ b/hw/mips/cputimer.c > >> @@ -25,21 +25,13 @@ > >> #include "qemu/timer.h" > >> #include "sysemu/kvm.h" > >> > >> -#define TIMER_FREQ 100 * 1000 * 1000 > >> - > >> -/* XXX: do not use a global */ > >> +/* Generate a random TLB index. > >> + * Skip wired entries. */ > >> uint32_t cpu_mips_get_random (CPUMIPSState *env) > >> { > >> - static uint32_t lfsr = 1; > >> - static uint32_t prev_idx = 0; > >> - uint32_t idx; > >> - /* Don't return same value twice, so get another value */ > >> - do { > >> - lfsr = (lfsr >> 1) ^ (-(lfsr & 1u) & 0xd0000001u); > >> - idx = lfsr % (env->tlb->nb_tlb - env->CP0_Wired) + env->CP0_Wired; > >> - } while (idx == prev_idx); > >> - prev_idx = idx; > >> - return idx; > >> + env->CP0_Random = env->CP0_Wired + > >> + random() % (env->tlb->nb_tlb - env->CP0_Wired); > >> + return env->CP0_Random; > >> } > >> > >> /* MIPS R4K timer */ > > > > Can you please give us more details about what issue you are trying to > > fix there? Especially I don't understand about the "skip wired entries" > > part. It seems the original code handles the wired entries correctly, > > and at least your patch doesn't seem to change anything regarded that > > part. > > The original code looks fine by itself. But when you try to run in for > nb_tlb=16 and CP0_Wired=1, you get a sequence: > > 15, 6, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, 2, 7, > 2, 7, 2, 7, 2... > > This is what happens when 4.4bsd kernel starts on pic32mz processor. > It makes the VM subsystem a bit crazy. Later the sequence becomes > better, but I think it makes sense to improve it somehow. > Thanks for the explanation, I know understand the issue and I agree it should be fixed.
diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c index 4f02a9f..94a29df 100644 --- a/hw/mips/cputimer.c +++ b/hw/mips/cputimer.c @@ -25,21 +25,13 @@ #include "qemu/timer.h" #include "sysemu/kvm.h" -#define TIMER_FREQ 100 * 1000 * 1000 - -/* XXX: do not use a global */ +/* Generate a random TLB index. + * Skip wired entries. */ uint32_t cpu_mips_get_random (CPUMIPSState *env) { - static uint32_t lfsr = 1; - static uint32_t prev_idx = 0; - uint32_t idx; - /* Don't return same value twice, so get another value */ - do { - lfsr = (lfsr >> 1) ^ (-(lfsr & 1u) & 0xd0000001u); - idx = lfsr % (env->tlb->nb_tlb - env->CP0_Wired) + env->CP0_Wired; - } while (idx == prev_idx); - prev_idx = idx; - return idx; + env->CP0_Random = env->CP0_Wired + + random() % (env->tlb->nb_tlb - env->CP0_Wired); + return env->CP0_Random; } /* MIPS R4K timer */
Signed-off-by: Serge Vakulenko <serge.vakulenko@gmail.com> --- hw/mips/cputimer.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)