linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	"H . Peter Anvin" <hpa@zytor.com>, Paul Turner <pjt@google.com>,
	linux-api@vger.kernel.org, Christian Brauner <brauner@kernel.org>,
	David.Laight@ACULAB.COM, carlos@redhat.com,
	Peter Oskolkov <posk@posk.io>,
	Alexander Mikhalitsyn <alexander@mihalicyn.com>
Subject: Re: [PATCH v4 01/25] rseq: Introduce feature size and alignment ELF auxiliary vector entries
Date: Tue, 18 Oct 2022 15:00:01 -0400	[thread overview]
Message-ID: <40d75f05-ef87-d64d-2e4a-60066e49f265@efficios.com> (raw)
In-Reply-To: <87fsfli1r9.fsf@oldenburg.str.redhat.com>

On 2022-10-18 11:34, Florian Weimer wrote:
> * Mathieu Desnoyers:
> 
>> If we extend struct rseq to a size that makes the compiler use an
>> alignment larger than 32 bytes in the future, and if the compiler uses
>> that larger alignment knowledge to issue instructions that require the
>> larger alignment, then it would be incorrect for user-space to
>> allocate the struct rseq on an alignment lower than the required
>> alignment.
>>
>> Indeed, on rseq registration, we have the following check:
>>
>> if (!IS_ALIGNED((unsigned long)rseq, __alignof__(*rseq))
>> [...]
>>     return -EINVAL;
>>
>> Which would break if the size of struct rseq is large enough that the
>> alignment grows larger than 32 bytes.
> 
> I never quite understood the reason for that check, it certainly made
> the glibc implementation more complicated.  But to support variable
> sizes internally, we'll have to put in some extra effort anyway, so that
> it won't matter much in the end.  As long as the required alignment
> isn't larger than the page size. 8-/

I don't expect it to grow so large.

There is one more reason why increasing the alignment of struct rseq may 
become useful as the structure grows: it would guarantee that it fits in 
a single lower level cache line as its size increases. It's not 
something I expect would break if not properly aligned, but it's a nice 
optimization.

I see two possible approaches here:

1) We expose the rseq alignment explicitly through auxv, and we can keep 
the IS_ALIGNED validation on rseq registration. This "IS_ALIGNED" check 
would probably have to be tweaked though, because if the registered
rseq size is 32, then an alignment of 32 is all we require. It's only if 
the rseq_len is different from 32 that we need to validate that the 
alignment matches the alignment of struct rseq.

2) We don't expose the rseq alignment through auxv, effectively fixing 
it at 32. We would need to modify the IS_ALIGNED check on rseq 
registration so it validates an alignment of 32 rather than using the 
alignment of struct rseq.

> 
>> You mentioned we could steal some high bits from AT_RSEQ_FEATURE_SIZE
>> to put the alignment. What is the issue with exposing an explicit
>> AT_RSEQ_ALIGN ? It's just a auxv entry, so I don't see it as a huge
>> performance concern to access 2 entries rather than one.
> 
> I don't mind too much, we already have a large on-stack array in the
> loader so that we can decode the auxiliary vector without a humongous
> switch statement.  But eventually that approach will stop working if the
> set of interesting AT_* values become too large and discontinuous.

OK. So I guess the main question here is whether we want fixed-32-bytes 
alignment, or do we want to be able to increase the mandated alignment 
in the future as struct rseq expands ?

The possible reasons for increasing the alignment over 32-bytes would be:

- Unforeseen compiler requirement on a structure alignment larger than 
32-bytes as we extend the size of struct rseq.
- Optimization to fit within a single LLC cache line as struct rseq grows.

Thoughts ?

Thanks,

Mathieu

> 
> Thanks,
> Florian
> 

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


  reply	other threads:[~2022-10-18 19:00 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 10:59 [PATCH v4 00/25] RSEQ node id and virtual cpu id extensions Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 01/25] rseq: Introduce feature size and alignment ELF auxiliary vector entries Mathieu Desnoyers
2022-10-10 12:42   ` Florian Weimer
2022-10-17 16:09     ` Mathieu Desnoyers
2022-10-17 17:32       ` Mathieu Desnoyers
2022-10-18 15:34         ` Florian Weimer
2022-10-18 19:00           ` Mathieu Desnoyers [this message]
2022-09-22 10:59 ` [PATCH v4 02/25] rseq: Introduce extensible rseq ABI Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 03/25] rseq: Extend struct rseq with numa node id Mathieu Desnoyers
2022-09-23 11:13   ` Peter Zijlstra
2022-09-23 13:00     ` Mathieu Desnoyers
2022-09-23 13:09     ` [PATCH v4.1 03/25 1/1] " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 04/25] selftests/rseq: Use ELF auxiliary vector for extensible rseq Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 05/25] selftests/rseq: Implement rseq numa node id field selftest Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 06/25] lib: Invert _find_next_bit source arguments Mathieu Desnoyers
2022-09-27  8:04   ` kernel test robot
2022-09-22 10:59 ` [PATCH v4 07/25] lib: Implement find_{first,next}_{zero,one}_and_zero_bit Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 08/25] cpumask: Implement cpumask_{first,next}_{zero,one}_and_zero Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 09/25] sched: Introduce per memory space current virtual cpu id Mathieu Desnoyers
2022-09-27 13:43   ` [PATCH v4.1 " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 10/25] rseq: Extend struct rseq with per memory space vcpu id Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 11/25] selftests/rseq: Remove RSEQ_SKIP_FASTPATH code Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 12/25] selftests/rseq: Implement rseq vm_vcpu_id field support Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 13/25] selftests/rseq: x86: Template memory ordering and percpu access mode Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 14/25] selftests/rseq: arm: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 15/25] selftests/rseq: arm64: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 16/25] selftests/rseq: mips: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 17/25] selftests/rseq: ppc: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 18/25] selftests/rseq: s390: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 19/25] selftests/rseq: riscv: " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 20/25] selftests/rseq: Implement basic percpu ops vm_vcpu_id test Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 21/25] selftests/rseq: Implement parametrized " Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 22/25] selftests/rseq: x86: Implement rseq_load_u32_u32 Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 23/25] selftests/rseq: Implement numa node id vs vm_vcpu_id invariant test Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 24/25] selftests/rseq: parametrized test: Report/abort on negative cpu id Mathieu Desnoyers
2022-09-22 10:59 ` [PATCH v4 25/25] tracing/rseq: Add mm_vcpu_id field to rseq_update Mathieu Desnoyers
2022-09-22 15:14   ` kernel test robot
2022-09-22 15:33     ` [PATCH v4.1 " Mathieu Desnoyers
2022-09-23  9:55   ` [PATCH v4 " kernel test robot
     [not found] ` <e753568d-599c-d81a-8456-085bbbb0264d@efficios.com>
     [not found]   ` <CAEE+ybnLUHjU5-dWcWgcWiq-AM4ocquSbZ=PWiuexEsPB8P5Gw@mail.gmail.com>
2022-09-23 13:46     ` [PATCH v4 00/25] RSEQ node id and virtual cpu id extensions Mathieu Desnoyers
2022-10-10 13:04 ` Florian Weimer
2022-10-17 16:05   ` Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=40d75f05-ef87-d64d-2e4a-60066e49f265@efficios.com \
    --to=mathieu.desnoyers@efficios.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=alexander@mihalicyn.com \
    --cc=boqun.feng@gmail.com \
    --cc=brauner@kernel.org \
    --cc=carlos@redhat.com \
    --cc=fweimer@redhat.com \
    --cc=hpa@zytor.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=pjt@google.com \
    --cc=posk@posk.io \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).