In D15147#319660, @jhb wrote:

In D15147#319396, @jmg wrote:

I have an spare BBB, and I can test this if you send me an image + test case. I'm not setup for building images right now.

The code looks correct, sans all the type changes. I'd recommend committing the type changes separately from fcmpset change.

The BBB has an armv7 CPU so it uses atomic-v6.h, not this file. Only a few specific armv4/v5 boards are supported by FreeBSD and they aren't the common BBB, ${FRUIT}Pi type boards which are all based on armv6 (original RPi) or armv7/v8.

I have an spare BBB, and I can test this if you send me an image + test case. I'm not setup for building images right now.

I only briefly reviewed changes, and I don't see any issues. I did not do a security review.

I don't see an issue with this change, looks good to me.

Are you sure this is even a problem? You have to enable FS_ATIME harvest flag before it'll even be a problem, from sys/dev/random/random_harvestq.c https://svnweb.freebsd.org/base/head/sys/dev/random/random_harvestq.c?annotate=324394#l456 :

if (!(harvest_context.hc_source_mask & (1 << origin)))
        return;

In D12451#258622, @cem wrote:

In D12451#258497, @jmg wrote:

It would be good to get a proper errors section added to the documentation.

Do you consider that an essential part of this change, or could that go in as a separate revision?

Only got part way through the patch. I'll continue reviewing it another time.

It would be good to get a proper errors section added to the documentation.

In D12452#258449, @jhb wrote:

On a general note, I would almost consider making this part of aesni.ko instead if the effective sets of CPUs that support both instruction sets is the same (or nearly so). In particular, the existing aesni.ko code does not accelerate things that combine AES with an HMAC (like an IPSec session using AES-CBC with a SHA HMAC). Having a single module that registers support for whatever algorithms the CPU supports (only SHA if AESNI isn't available for example) would permit the driver to handle these chained operations by combining AESNI with accelerated SHA hashing. (In this case aesni.ko would perhaps be better named "x86_crypto.ko" or some such to mean "accelerated software crypto for x86".)

I don't see any man page updates for this code. Please make sure any new capabilities, flags and features are properly documented in crypto(9). I cannot provide review on the patch till documentation is written.

So, you'll still suffer terribly with this, as random_harvest_fast is not PCPU, so you'll have the cache line bouncing around.

looks fine, have you verified that the tests in tests/sys/opencrypto pass? they are not present in your test plan.

In D10384#215987, @emeric.poupon_stormshield.eu wrote:

Eventually the goal for us is to use crypto(9) from IPsec to accelerate single flows processing. Indeed IPsec does not guarantee packet ordering (neither does IP), but it would be for sure quite harmful for some end user applications if packets are not ordered.
A same crypto session may be used for several flows coming from the nic on several CPUs. It would be needed to keep the packets ordered for each flow on each CPU but it does not really matter to loss the ESP packet order in ouput, as the anti replay window handles that on the remote host.
That's why I think it would be nice for crypto(9) users to keep ordering when dispatching the jobs.

No, this is a requirement of the IPsec layer, not all users of crypto(9) require this. For example, disk encryption does not need this, as the upper layers ensures that writes are ordered correctly (ZFS and UFS both do this). And by forcing order, you are increasing latency unnecessarily for other consumers.

This isn't hard to handle at the IPsec layer. You use a TAILQ to enqueue the packets w/ a simple data structure w/ a flag that gets set when the packet is completed, then each completed packet, while the head of the tailq is ready, send it. It's not hard, and keeps the ordering logic where it belongs, or you add a flag to crypto(9) and the logic there, but you need to allow non-ordered operation.

This is maybe a bit more difficult, since we would need to reorder packets only within the flows that may share the same crypto session, but I get your idea. Maybe a reording queue per CPU would do the job, since we expect each flow to be pinned on the same CPU.

If this is truly a PRNG which it appears it is, It is not an effective source of entropy and should not be added. I'd be happy to review more information on the PRNG if you have it.

In D10384#215486, @emeric.poupon_stormshield.eu wrote:

In D10384#215403, @jmg wrote:

as per other comments in the code, ordering does not have to be maintained.. w/ the async nature of callbacks, it is already assumed that the callers can handle this.

Thanks for your comments!

Eventually the goal for us is to use crypto(9) from IPsec to accelerate single flows processing. Indeed IPsec does not guarantee packet ordering (neither does IP), but it would be for sure quite harmful for some end user applications if packets are not ordered.
A same crypto session may be used for several flows coming from the nic on several CPUs. It would be needed to keep the packets ordered for each flow on each CPU but it does not really matter to loss the ESP packet order in ouput, as the anti replay window handles that on the remote host.
That's why I think it would be nice for crypto(9) users to keep ordering when dispatching the jobs.

as per other comments in the code, ordering does not have to be maintained.. w/ the async nature of callbacks, it is already assumed that the callers can handle this.

markm, I've pointed out where the issue is.

This will cause issues on platforms that do not use loader. We do not require loader on all of our platforms, and those that don't will have issues w/ the way chacha is started. As there is not an error (continues), this creates divergent behavior.

• add /boot/entropy to install generation.. Also, be extra careful
• add quotes around the $i for more space protection..
• use umask per delphij...
• dteske says the parens are not needed.. remove them..
• pull in dteske's version..

• add /boot/entropy to install generation.. Also, be extra careful
• add quotes around the $i for more space protection..
• use umask per delphij...
• dteske says the parens are not needed.. remove them..

take back so I can update and confirm my change is same..

has this been tested? This looks good otherwise.

mark various parts done

• add /boot/entropy to install generation.. Also, be extra careful
• add quotes around the $i for more space protection..
• use umask per delphij...

for jails we don't need to create the file, but I don't see any harm in having this file created... I looked briefly at the jail script, and I don't see a good way to turn it off in the jail case...

I have no issue w/ the kern_event.c change. I am have not reviewed anything else.

though using dd to put it in an image works, it doesn't change the file size, so I needed to delete some comments, and add white space... once I did that, I was able to verify that the script ran fine...

• add /boot/entropy to install generation.. Also, be extra careful
• add quotes around the $i for more space protection..

Well, I attempted to test this, and I don't believe that this file is being called at the end of install.

I had thought that we weren't writing out even /entropy files, but @op pointed me to this file.

address some comments.

Just a few comments.

I said Yes, but I think that we need to clean up our base libraries first. We need to make most/all libs private. If it isn't part of posix, it needs a man page which links to functions provided.

Thanks. changed.

• change a needs to to a must to be more correct.. If there are threads

Address all but /The/That/ change.. I understand why you suggested it, but it reads not as easily for me... We are only referencing the once example.

• address comments from bjk...

hopefully update the diff w/ complete change..

ok, this is an issue w/ git and arc locally, not with phabric...

• Fix date..

add documentation to kthread, and fix up kproc Sx

Minor comments.

Thanks, looks good.

In D3354#68427, @markm wrote:

In D3354#68413, @delphij wrote:

I have only glanced over the code and it seems good overall.

Thanks!

One thought: can we make DEV_RANDOM something like NO_DEV_RANDOM? That way it would be an opt-out feature, as most systems still need it.

Nope, sorry. The DEV_RANDOM pseudo-option comes for free from 'device random' in the kernel config file. There is no negative variant, but it is safe enough given that the kernel configs that need this device have it.

Are you opening this up to more reviewers? Or still only for SO (aka delphij)? The reason I ask is that secteam only consists of delphij, so adding secteam doesn't add any new reviewers.

Comments are not required to be addressed.

Look good.. Please address the comments.

In D3303#66772, @ed wrote:

In D3303#66768, @jmg wrote:

I don't see an issue w/ this, though if this is committed, but then CloudABI decided to not use it anymore, I request that it be removed.

Of course. Would it make sense to #ifdef _KERNEL it and revert the man page changes? That way we can be certain that userspace won't use it.

I don't see an issue w/ this, though if this is committed, but then CloudABI decided to not use it anymore, I request that it be removed.

I'm fine w/ this change.

Please post a complete patch that includes both sets of changes, and adds proper documentation.

errx needed to be used instead of err as errno will not be set in this case.

Update to add error when d == 0...

I could also use the term reciprocal, but for me invert makes sense and is more common.

In D3084#61045, @imp wrote:

Invert isn't the right terminology.
You are converting from the time domain to the frequency domain.
This is a fundamental conceptual faux pas.

Other than the set/true change, the changes to the manpage are great.