On 03/01/2026 10:46, David Laight wrote: > On Fri, 2 Jan 2026 14:09:26 +0000 > Ryan Roberts <[email protected]> wrote: > >> On 02/01/2026 13:39, Jason A. Donenfeld wrote: >>> Hi Ryan, >>> >>> On Fri, Jan 2, 2026 at 2:12 PM Ryan Roberts <[email protected]> wrote: >>>> context. Given the function is just a handful of operations and doesn't >>> >>> How many? What's this looking like in terms of assembly? >> >> 25 instructions on arm64: >> >> 0000000000000000 <prandom_u32_state>: >> 0: 29401403 ldp w3, w5, [x0] >> 4: aa0003e1 mov x1, x0 >> 8: 29410002 ldp w2, w0, [x0, #8] >> c: 531e74a4 lsl w4, w5, #2 >> 10: 530e3468 lsl w8, w3, #18 >> 14: 4a0400a5 eor w5, w5, w4 >> 18: 4a031863 eor w3, w3, w3, lsl #6 >> 1c: 53196047 lsl w7, w2, #7 >> 20: 53134806 lsl w6, w0, #13 >> 24: 4a023442 eor w2, w2, w2, lsl #13 >> 28: 4a000c00 eor w0, w0, w0, lsl #3 >> 2c: 121b6884 and w4, w4, #0xffffffe0 >> 30: 120d3108 and w8, w8, #0xfff80000 >> 34: 121550e7 and w7, w7, #0xfffff800 >> 38: 120c2cc6 and w6, w6, #0xfff00000 >> 3c: 2a456c85 orr w5, w4, w5, lsr #27 >> 40: 2a433504 orr w4, w8, w3, lsr #13 >> 44: 2a4254e3 orr w3, w7, w2, lsr #21 >> 48: 2a4030c2 orr w2, w6, w0, lsr #12 >> 4c: 4a020066 eor w6, w3, w2 >> 50: 4a050080 eor w0, w4, w5 >> 54: 4a0000c0 eor w0, w6, w0 >> 58: 29001424 stp w4, w5, [x1] >> 5c: 29010823 stp w3, w2, [x1, #8] >> 60: d65f03c0 ret > > That is gcc, clang seems to generate something horrid (from godbolt). > I'm not sure what it has tried to do (and maybe it can't in kernel) > but it clearly doesn't help! > .LCPI0_0: > .word 18 > .word 2 > .word 7 > .word 13 > .LCPI0_1: > .word 6 > .word 2 > .word 13 > .word 3 > .LCPI0_2: > .word 4294443008 > .word 4294967264 > .word 4294965248 > .word 4293918720 > .LCPI0_3: > .word 4294967283 > .word 4294967269 > .word 4294967275 > .word 4294967284 > prandom_u32_state: > adrp x9, .LCPI0_1 > ldr q0, [x0] > adrp x10, .LCPI0_3 > ldr q1, [x9, :lo12:.LCPI0_1] > adrp x9, .LCPI0_0 > ldr q3, [x10, :lo12:.LCPI0_3] > ldr q2, [x9, :lo12:.LCPI0_0] > adrp x9, .LCPI0_2 > mov x8, x0 > ushl v1.4s, v0.4s, v1.4s > ushl v2.4s, v0.4s, v2.4s > eor v0.16b, v1.16b, v0.16b > ldr q1, [x9, :lo12:.LCPI0_2] > and v1.16b, v2.16b, v1.16b > ushl v0.4s, v0.4s, v3.4s > orr v0.16b, v0.16b, v1.16b > ext v1.16b, v0.16b, v0.16b, #8 > str q0, [x8] > eor v1.8b, v0.8b, v1.8b > fmov x9, d1 > lsr x10, x9, #32 > eor w0, w9, w10 > ret > > The x86 versions are a little longer (arm's barrel shifter helps a lot). > >> >>> It'd also be >>> nice to have some brief analysis of other call sites to have >>> confirmation this isn't blowing up other users. >> >> I compiled defconfig before and after this patch on arm64 and compared the >> text >> sizes: >> >> $ ./scripts/bloat-o-meter -t vmlinux.before vmlinux.after >> add/remove: 3/4 grow/shrink: 4/1 up/down: 836/-128 (708) >> Function old new delta >> prandom_seed_full_state 364 932 +568 >> pick_next_task_fair 1940 2036 +96 >> bpf_user_rnd_u32 104 196 +92 >> prandom_bytes_state 204 260 +56 >> e843419@0f2b_00012d69_e34 - 8 +8 >> e843419@0db7_00010ec3_23ec - 8 +8 >> e843419@02cb_00003767_25c - 8 +8 >> bpf_prog_select_runtime 448 444 -4 >> e843419@0aa3_0000cfd1_1580 8 - -8 >> e843419@0aa2_0000cfba_147c 8 - -8 >> e843419@075f_00008d8c_184 8 - -8 >> prandom_u32_state 100 - -100 >> Total: Before=19078072, After=19078780, chg +0.00% >> >> So 708 bytes more after inlining. > > Doesn't look like there are many calls. > >> The main cost is prandom_seed_full_state(), >> which calls prandom_u32_state() 10 times (via prandom_warmup()). I expect we >> could turn that into a loop to reduce ~450 bytes overall. > > That would always have helped the code size. > And I suspect the other costs of that code make unrolling the loop pointless. > >> >> I'm not really sure if 708 is good or bad... >> >>> >>>> +static __always_inline u32 prandom_u32_state(struct rnd_state *state) >>> >>> Why not just normal `inline`? Is gcc disagreeing with the inlinability >>> of this function? >> >> Given this needs to be called from a noinstr function, I didn't want to give >> the >> compiler the opportunity to decide not to inline it, since in that case, some >> instrumentation might end up being applied to the function body which would >> blow >> up when called in the noinstr context. >> >> I think the other 2 options are to keep prandom_u32_state() in the c file but >> mark it noinstr or rearrange all the users so that thay don't call it until >> instrumentation is allowable. The latter is something I was trying to avoid. >> >> There is some previous discussion of this at [1]. >> >> [1] https://lore.kernel.org/all/aS65LFUfdgRPKv1l@J2N7QTR9R3/ >> >> Perhaps keeping prandom_u32_state() in the c file and making it noinstr is >> the >> best compromise? > > Or define prandom_u32_state_inline() as always_inline and have the > real function: > u32 prandom_u32_state(struct rnd_state *state) > { > return prandom_u32_state_inline(state); > } > > So that the callers can pick the inline version if it really matters.
Ahh yes, that sounds like the simplest/best idea to me. I'll take this approach for the next version assuming Jason is ok with it? Thanks, Ryan > > David > >> >> Thanks, >> Ryan >> >>> >>> Jason >> >> >
