Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 7 Feb 2022 11:39:54 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        paulmck <paulmck@kernel.org>, Boqun Feng <boqun.feng@gmail.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Paul Turner <pjt@google.com>,
        linux-api <linux-api@vger.kernel.org>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Florian Weimer <fw@deneb.enyo.de>,
        David Laight <David.Laight@aculab.com>,
        carlos <carlos@redhat.com>, Peter Oskolkov <posk@posk.io>,
        libc-coord <libc-coord@lists.openwall.com>
Message-ID: <1069583975.46528.1644251994113.JavaMail.zimbra@efficios.com>
In-Reply-To: <YgBDDVZSZqID8L3R@hirez.programming.kicks-ass.net>
References: <20220203193853.21511-1-mathieu.desnoyers@efficios.com> <20220203193853.21511-3-mathieu.desnoyers@efficios.com> <YgBDDVZSZqID8L3R@hirez.programming.kicks-ass.net>
Subject: Re: [RFC PATCH 3/3] rseq: extend struct rseq with numa node id
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: rseq: extend struct rseq with numa node id
Thread-Index: JygeOzRonTrMEViFglh4jrP23b2dow==
Precedence: bulk

----- On Feb 6, 2022, at 4:52 PM, Peter Zijlstra peterz@infradead.org wrote:

> On Thu, Feb 03, 2022 at 02:38:53PM -0500, Mathieu Desnoyers wrote:
>> +static int rseq_reset_rseq_cpu_node_id(struct task_struct *t)
>>  {
>> -	u32 cpu_id_start = 0, cpu_id = RSEQ_CPU_ID_UNINITIALIZED;
>> +	u32 cpu_id_start = 0, cpu_id = RSEQ_CPU_ID_UNINITIALIZED, node_id = 0;
>>  
>>  	/*
>>  	 * Reset cpu_id_start to its initial state (0).
>> @@ -124,6 +126,11 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
>>  	 */
>>  	if (put_user(cpu_id, &t->rseq->cpu_id))
>>  		return -EFAULT;
>> +	/*
>> +	 * Reset node_id to its initial state (0).
>> +	 */
>> +	if (put_user(node_id, &t->rseq->node_id))
>> +		return -EFAULT;
> 
> Why 0 vs -1 ?

That's because we are adding this field as a replacement of 4 from the 12 bytes
of padding at the end of the original struct rseq uapi. I expect this padding to be
zero-initialized in glibc, but I don't think its initial value matters very much.

The initial value for cpu_id (-1) mattered  because that field was used to check whether
rseq was initialized. It should not matter as much for extended fields. Taking the
glibc userspace ABI into account, I would expect the following for node_id feature
discovery by applications:

- use __rseq_size (symbol exposed by glibc) to validate whether rseq is registered for
  the process.
- use getauxval(AT_RSEQ_FEATURE_SIZE) to know how many rseq fields are populated by the
  kernel.

So there are really only a few possible scenarios here:

- __rseq_size == 0: rseq registration unsuccessful
- else:
  - getauxval(AT_RSEQ_FEATURE_SIZE) == 0:
    - Kernel only implements the features of original rseq (last populated field is "flags",
      offsetofend(, flags) == 20).
  - else if getauxval(AT_RSEQ_FEATURE_SIZE) <= 32:
    - all exposed kernel features can be used, because the original rseq len was 32 bytes,
      so glibc always registers at least a 32-byte area.
  - else
    - only min(__rseq_size, getauxval(AT_RSEQ_FEATURE_SIZE)) features can be used.
      glibc is required to allocate a memory area large enough to hold all features,
      except when it allocates the original uapi 32 bytes.

So until we reach the 32 bytes limit of the original rseq structure, applications can directly
use getauxval() to use the additional features before glibc is made aware of them. From that
point on, glibc needs to allocate a larger memory area to hold those features.

I suspect that future glibc versions might want to expose a new "__rseq_feature_size"
symbol which contains the result of

    min(__rseq_size, getauxval(AT_RSEQ_FEATURE_SIZE) ? : offsetofend(, flags))

so applications don't have to do this computation on their own.

Considering that applications will likely check for rseq registration and feature availability
by checking offsetofend(, field) <= __rseq_feature_size, then I don't think the initialization
value matters much.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com