random(4): Poll for signals during large reads

[cem]

Summary

Occasionally poll for signals during large reads of the /dev/u?random
devices. This allows cancellation via SIGINT of accidental invocations of
very large reads. (A 2GB /dev/random read can be aborted.)

I believe this behavior was intended since 2014 (r273997), just not fully
implemented.

Test Plan

Related to, but not a dependency on or of D14500 .

[cem]

Ping — this one is pretty short and sweet. I'd like to get this in before the other change, if there's no objection.

[delphij]

Oops, I thought I have send out the comment but turns out I didn't.

sys/dev/random/randomdev.c

133 ↗

(On Diff #40258)

Probably a good idea to comment with reasoning (e.g. this can be done in below 0.X seconds on modern CPU, etc.).

177 ↗

(On Diff #40258)

Not a native speaker but "every few MB" sounded weird to me, should it be "every a few MiBs"? (It should be MiB instead of MB, see https://en.wikipedia.org/wiki/Mebibyte ).

181 ↗

(On Diff #40258)

Could you please change the code so we avoid using division here? e.g.:

(total_read ^ (total_read + c) & ~(sigcheck_period -1)) != 0, or change sigcheck_period to a mask (e.g. ~(1 << 24 - 1))?

185 ↗

(On Diff #40258)

My recommendation is to move this to right after uiomove() because they belong to the same logical block.

188 ↗

(On Diff #40258)

I think the code expectation is that, before going to tsleep_sbt() we have read something. Maybe move this a little bit earlier, right after total_read adjustment was done, unconditionally?

190 ↗

(On Diff #40258)

The comment is no longer accurate (it's possible that we returns 0 even when a signal is delivered to the calling thread and it's not necessarily a partial read, for example, it we could have finished reading the last page). Maybe change it to "Some data transferred".

[cem]

In D14684#309026, @delphij wrote:

Oops, I thought I have send out the comment but turns out I didn't.

Ah, Phabricator strikes again :-).

sys/dev/random/randomdev.c

133 ↗

(On Diff #40258)

Sure, will do.

177 ↗

(On Diff #40258)

I think you're right about appending an 's' for plural. https://english.stackexchange.com/questions/503/what-is-the-correct-way-to-pluralize-an-acronym

I don't think the distinction between MB and MiB matters given the non-specificity of "few." (1 MB is 0.954 MiB. What is 95.4% of "a few?" Still "a few.") Can change that if you want.

181 ↗

(On Diff #40258)

I would prefer to leave it expressed as division, as that clearly shows the intent of the logical operation to the reader.

I would guess that a compiler can optimize this into the same code suggested by the bitwise construction you suggest — so we would be doing the compiler's work for it at the cost of clarity to the reader.

Even if the compiler can't optimize it automatically, I'm hesitant to replace the clear version of the code unless the spot shows up in profiling. Back of the napkin math suggests urandom generation takes about 17 cycles per byte (cpb) on my machine. How expensive is an integer division? 100 cycles on a high clock deep pipeline x86? Even if it's that expensive (and I don't think it is), that only adds 0.04 cpb, or around 0.2% (amortized over PAGE_SIZE).

Anyway, Clang optimizes this to a right shift, so perhaps the issue is moot:

181                             if (error == 0 &&
   0x00000000000005b9 <+393>:   mov    %rbx,%r11

182                                 total_read / sigchk_period !=
   0x00000000000005bc <+396>:   add    %r15,%rbx
   0x00000000000005bf <+399>:   mov    %rbx,%rcx
   0x00000000000005c2 <+402>:   shr    $0x18,%r11
   0x00000000000005c6 <+406>:   shr    $0x18,%rcx
   0x00000000000005ca <+410>:   cmp    %rcx,%r11
   0x00000000000005cd <+413>:   jne    0x628 <read_random_uio+504>

(0x18 == 24 == 16 MiB.)

185 ↗

(On Diff #40258)

Sure, will do.

188 ↗

(On Diff #40258)

Maybe we should just remove it? It becomes really obviously tautological when moved up as suggested.

190 ↗

(On Diff #40258)

Maybe we should skip the sleep and return immediately if all bytes have already been read? Then we would never hit this condition for full reads anyway.

[cem]

Incorporate delphij's feedback:

• Add comment justifying arbitrary signal check interval
• Move total_read increment to right under uiomove
• Spell MB with explicit plural
• Only sleep if there is more data to read
• Remove useless total_read KASSERT

[delphij]

sys/dev/random/randomdev.c

183 ↗

(On Diff #40336)

Since the total_read is now the after read total, I think you could just check if (total_read % sigchk_period == 0) now, instead of compensating the add? When we reached here we know we have read at least 'c' bytes (non-zero). I bet a modern compiler can easily optimize that into an & operation.

Speaking for the "skip sleep immediately" (below, but I think you are talking about this context since uio->uio_resid != 0 is added here), I'm fine with either way. Personally, I don't see a reason to check it since the quantity is sufficiently large and the sleep time is sufficiently small, though.

188 ↗

(On Diff #40258)

Style nit: Note that the { } s are not needed now.

Yes I think the comment is now redundant, the code is quite self explanatory now.

[cem]

sys/dev/random/randomdev.c

183 ↗

(On Diff #40336)

Since the total_read is now the after read total, I think you could just check if (total_read % sigchk_period == 0) now, instead of compensating the add? When we reached here we know we have read at least 'c' bytes (non-zero).

Sure, I think that's fine as long as sigchk_period % PAGE_SIZE == 0 (it is, as PAGE_SIZE is a smaller power of two, but maybe we could add an invariant to make that explicit).

188 ↗

(On Diff #40258)

Ok, will clean up.

[cem]

Update with feedback from delphij:

• Use simpler boundary check for when we should poll for signals (sleep), and add compile-time invariant for the math assumption made.
• Drop excess { } and comment.

[delphij]

LGTM, thanks!

[delphij]

Just in case an explicit blessing from secteam is needed.

[cem]

Sure, thanks for reviewing!