lkml.org 
[lkml]   [2023]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 1/4] mm/ksm: add ksm advisor
    From
    [...]

    > +
    > +/**
    > + * struct advisor_ctx - metadata for KSM advisor
    > + * @start_scan: start time of the current scan
    > + * @scan_time: scan time of previous scan
    > + * @change: change in percent to pages_to_scan parameter
    > + * @cpu_time: cpu time consumed by the ksmd thread in the previous scan
    > + */
    > +struct advisor_ctx {
    > + ktime_t start_scan;
    > + unsigned long scan_time;
    > + unsigned long change;
    > + unsigned long long cpu_time;
    > +};
    > +static struct advisor_ctx advisor_ctx;
    > +
    > +/* Define different advisor's */
    > +enum ksm_advisor_type {
    > + KSM_ADVISOR_NONE,
    > + KSM_ADVISOR_SCAN_TIME,
    > +};
    > +static enum ksm_advisor_type ksm_advisor;
    > +
    > +static void init_advisor(void)
    > +{
    > + advisor_ctx = (const struct advisor_ctx){ 0 };
    > +}

    Again, you can drop this completely. The static values are already
    initialized to 0.

    Or is there any reason to initialize to 0 explicitly?

    > +
    > +static void set_advisor_defaults(void)
    > +{
    > + if (ksm_advisor == KSM_ADVISOR_NONE)
    > + ksm_thread_pages_to_scan = DEFAULT_PAGES_TO_SCAN;
    > + else if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
    > + ksm_thread_pages_to_scan = ksm_advisor_min_pages;
    > +}

    That function is unused?

    > +
    > +static inline void advisor_start_scan(void)
    > +{
    > + if (ksm_advisor == KSM_ADVISOR_SCAN_TIME)
    > + advisor_ctx.start_scan = ktime_get();
    > +}
    > +
    > +static inline s64 advisor_stop_scan(void)
    > +{
    > + return ktime_ms_delta(ktime_get(), advisor_ctx.start_scan);
    > +}

    Just inline that into the caller. Then rename run_advisor() into
    advisor_stop_scan(). So in scan_get_next_rmap_item)( you have paired
    start+stop hooks.

    > +
    > +/*
    > + * Use previous scan time if available, otherwise use current scan time as an
    > + * approximation for the previous scan time.
    > + */
    > +static inline unsigned long prev_scan_time(struct advisor_ctx *ctx,
    > + unsigned long scan_time)
    > +{
    > + return ctx->scan_time ? ctx->scan_time : scan_time;
    > +}
    > +
    > +/* Calculate exponential weighted moving average */
    > +static unsigned long ewma(unsigned long prev, unsigned long curr)
    > +{
    > + return ((100 - EWMA_WEIGHT) * prev + EWMA_WEIGHT * curr) / 100;
    > +}
    > +
    > +/*
    > + * The scan time advisor is based on the current scan rate and the target
    > + * scan rate.
    > + *
    > + * new_pages_to_scan = pages_to_scan * (scan_time / target_scan_time)
    > + *
    > + * To avoid pertubations it calculates a change factor of previous changes.

    s/pertubations/perturbations/

    Do you also want to describe how min/max CPU comes into play?

    > + * A new change factor is calculated for each iteration and it uses an
    > + * exponentially weighted moving average. The new pages_to_scan value is
    > + * multiplied with that change factor:
    > + *
    > + * new_pages_to_scan *= change facor
    > + *
    > + * In addition the new pages_to_scan value is capped by the max and min
    > + * limits.
    > + */



    With that, LGTM

    Acked-by: David Hildenbrand <david@redhat.com>

    --
    Cheers,

    David / dhildenb

    \
     
     \ /
      Last update: 2023-12-12 14:45    [W:5.895 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site