linux-kernel - Re: [PATCH v0.8 4/6] sched/umcg, lib/umcg: implement libumcg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFTs51X3s1zMVczUk_yHzLmLj5O+KaGmBy9go0EaVhphdNH_zg@mail.gmail.com>
Date:   Sun, 7 Nov 2021 10:27:02 -0800
From:   Peter Oskolkov <posk@...k.io>
To:     Tao Zhou <tao.zhou@...ux.dev>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>, linux-mm@...ck.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-api@...r.kernel.org, Paul Turner <pjt@...gle.com>,
        Ben Segall <bsegall@...gle.com>,
        Peter Oskolkov <posk@...gle.com>,
        Andrei Vagin <avagin@...gle.com>, Jann Horn <jannh@...gle.com>,
        Thierry Delisle <tdelisle@...terloo.ca>
Subject: Re: [PATCH v0.8 4/6] sched/umcg, lib/umcg: implement libumcg

On Sun, Nov 7, 2021 at 8:33 AM Tao Zhou <tao.zhou@...ux.dev> wrote:
>
> On Thu, Nov 04, 2021 at 12:58:02PM -0700, Peter Oskolkov wrote:
>
> > +/* Update the state variable, set new timestamp. */
> > +static bool umcg_update_state(uint64_t *state, uint64_t *prev, uint64_t next)
> > +{
> > +     uint64_t prev_ts = (*prev) >> (64 - UMCG_STATE_TIMESTAMP_BITS);
> > +     struct timespec now;
> > +     uint64_t next_ts;
> > +     int res;
> > +
> > +     /*
> > +      * clock_gettime(CLOCK_MONOTONIC, ...) takes less than 20ns on a
> > +      * typical Intel processor on average, even when run concurrently,
> > +      * so the overhead is low enough for most applications.
> > +      *
> > +      * If this is still too high, `next_ts = prev_ts + 1` should work
> > +      * as well. The only real requirement is that the "timestamps" are
> > +      * uniqueue per thread within a reasonable time frame.
> > +      */
> > +     res = clock_gettime(CLOCK_MONOTONIC, &now);
> > +     assert(!res);
> > +     next_ts = (now.tv_sec * NSEC_PER_SEC + now.tv_nsec) >>
> > +             UMCG_STATE_TIMESTAMP_GRANULARITY;
> > +
> > +     /* Cut higher order bits. */
> > +     next_ts &= ((1ULL << UMCG_STATE_TIMESTAMP_BITS) - 1);
>
> This is the right cut.. The same to the kernel side.

Yes, thanks!

>
> > +
> > +     if (next_ts == prev_ts)
> > +             ++next_ts;
> > +
> > +#ifndef NDEBUG
> > +     if (prev_ts > next_ts) {
> > +             fprintf(stderr, "%s: time goes back: prev_ts: %lu "
> > +                             "next_ts: %lu diff: %lu\n", __func__,
> > +                             prev_ts, next_ts, prev_ts - next_ts);
> > +     }
> > +#endif
> > +
> > +     /* Remove old timestamp, if any. */
> > +     next &= ((1ULL << (64 - UMCG_STATE_TIMESTAMP_BITS)) - 1);
> > +
> > +     /* Set the new timestamp. */
> > +     next |= (next_ts << (64 - UMCG_STATE_TIMESTAMP_BITS));
> > +
> > +     /*
> > +      * TODO: review whether memory order below can be weakened to
> > +      * memory_order_acq_rel for success and memory_order_acquire for
> > +      * failure.
> > +      */
> > +     return atomic_compare_exchange_strong_explicit(state, prev, next,
> > +                     memory_order_seq_cst, memory_order_seq_cst);
> > +}
> > +
>
> > +static void task_unlock(struct umcg_task_tls *task, uint64_t expected_state,
> > +             uint64_t new_state)
> > +{
> > +     bool ok;
> > +     uint64_t next;
> > +     uint64_t prev = atomic_load_explicit(&task->umcg_task.state_ts,
> > +                                     memory_order_acquire);
> > +
> > +     next = ((prev & ~UMCG_TASK_STATE_MASK_FULL) | new_state) & ~UMCG_TF_LOCKED;
>
> Use UMCG_TASK_STATE_MASK instead and the other state flag can be checked.

Why? We want to clear the TF_LOCKED flag and keep every other bit of
state, including other state flags (but excluding timestamp).


>
> All others places that use UMCG_TASK_STATE_MASK_FULL to mask to check
> the task state may seems reasonable if the state flag not allowed to
> be set when we check that task state, otherwise use UMCG_TASK_STATE_MASK
> will be enough.
>
> Not sure.
>
>
> Thanks,
> Tao
> > +     assert(next != prev);
> > +     assert((prev & UMCG_TASK_STATE_MASK_FULL & ~UMCG_TF_LOCKED) == expected_state);
> > +
> > +     ok = umcg_update_state(&task->umcg_task.state_ts, &prev, next);
> > +     assert(ok);
> > +}