Re: [Xen-devel] [PATCH RFC 04/25] Replace "/" operand with div64

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.11 at 19:19, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> ARM does not have a division operator: division has to be implemented
> in software.
> On x86 and ia64 div64 falls back to a do_div based implementation.

But this needs to be done carefully and correctly - there's no
one-fits-all div64() implementation.

> --- a/xen/common/sched_sedf.c
> +++ b/xen/common/sched_sedf.c
> @@ -127,7 +128,7 @@ struct sedf_cpu_info {
>  
>  #define PERIOD_BEGIN(inf) ((inf)->deadl_abs - (inf)->period)
>  
> -#define DIV_UP(x,y) (((x) + (y) - 1) / y)
> +#define DIV_UP(x,y) div64((x) + (y) - 1, y)

There are uses of this with y being s_time_t.

>  #define extra_runs(inf)      ((inf->status) & 6)
>  #define extra_get_cur_q(inf) (((inf->status & 6) >> 1)-1)
> @@ -678,8 +679,7 @@ static void desched_extra_dom(s_time_t now, struct vcpu 
> *d)
>            utilization and is used somewhat incremental!*/
>          if ( !inf->extraweight )
>              /*NB: use fixed point arithmetic with 10 bits*/
> -            inf->score[EXTRA_UTIL_Q] = (inf->period << 10) /
> -                inf->slice;
> +            inf->score[EXTRA_UTIL_Q] = div64(inf->period << 10, inf->slice);

inf->slice is s_time_t, i.e. 64-bit (and signed, but that's not relevant here).

>          else
>              /*conversion between realtime utilisation and extrawieght:
>                full (ie 100%) utilization is equivalent to 128 extraweight*/
> @@ -996,8 +996,8 @@ static void unblock_short_extra_support(
>    
>          if ( inf->short_block_lost_tot )
>          {
> -            inf->score[0] = (inf->period << 10) /
> -                inf->short_block_lost_tot;
> +            inf->score[0] = div64(inf->period << 10,
> +                                     inf->short_block_lost_tot);

inf->short_block_lost_tot is s_time_t, too.

>  #ifdef SEDF_STATS
>              inf->pen_extra_blocks++;
>  #endif
> @@ -1224,8 +1224,8 @@ static void sedf_dump_domain(struct vcpu *d)
>      
>  #ifdef SEDF_STATS
>      if ( EDOM_INFO(d)->block_time_tot != 0 )
> -        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->penalty_time_tot * 100) /
> -               EDOM_INFO(d)->block_time_tot);
> +        printk(" pen=%"PRIu64"%%",
> +               div64(EDOM_INFO(d)->penalty_time_tot * 100, 
> EDOM_INFO(d)->block_time_tot));

As is ->block_time_tot.

>      if ( EDOM_INFO(d)->block_tot != 0 )
>          printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
>                 "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
> @@ -1357,9 +1357,9 @@ static int sedf_adjust_weights(struct cpupool *c, 
> struct xen_domctl_scheduler_op
>                  /*check for overflows*/
>                  ASSERT((WEIGHT_PERIOD < ULONG_MAX) 
>                         && (EDOM_INFO(p)->slice_orig < ULONG_MAX));
> -                sumt[cpu] += 
> -                    (WEIGHT_PERIOD * EDOM_INFO(p)->slice_orig) / 
> -                    EDOM_INFO(p)->period_orig;
> +                sumt[cpu] += div64(
> +                    WEIGHT_PERIOD * EDOM_INFO(p)->slice_orig,
> +                    EDOM_INFO(p)->period_orig);

And ->period_orig.

>              }
>          }
>      }
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -503,16 +504,18 @@ static void timer_softirq_action(void)
>  
>  s_time_t align_timer(s_time_t firsttick, uint64_t period)
>  {
> +    uint64_t n;

    uint64_t n = firsttick - 1;

>      if ( !period )
>          return firsttick;
>  
> -    return firsttick + (period - 1) - ((firsttick - 1) % period);
> +    n = firsttick + (period - 1) - (firsttick - 1);
> +    return do_div(n, period);

    return firsttick + (period - 1) - do_div(n, period);

But again period is a 64-bit value, so using do_div() isn't correct
here.

>  }
>  
>  static void dump_timer(struct timer *t, s_time_t now)
> --- a/xen/include/asm-ia64/linux/asm-generic/div64.h
> +++ b/xen/include/asm-ia64/linux/asm-generic/div64.h
> @@ -55,4 +55,10 @@ extern uint32_t __div64_32(uint64_t *dividend, uint32_t 
> divisor);
>  
>  #endif /* BITS_PER_LONG */
>  
> +static inline uint64_t div64(uint64_t n, uint32_t base)

Return value type ought to be uint32_t for this particular flavor. More
flavors (64-bit divisor, signed divisor) may/will be needed.

> +{
> +     do_div(n, base);
> +     return n;
> +}
> +
>  #endif /* _ASM_GENERIC_DIV64_H */
> --- a/xen/include/asm-x86/div64.h
> +++ b/xen/include/asm-x86/div64.h
> @@ -46,4 +46,10 @@
>  
>  #endif
>  
> +static inline uint64_t div64(uint64_t n, uint32_t base)

Same here.

> +{
> +     do_div(n, base);
> +     return n;
> +}
> +
>  #endif

Did you really test this in its current shape on x86 (32-bit and 64-bit)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel