For reference, on an active pf firewall, before the patch, ~1.56% cpu core was used on the purge thread, after the patch, .427% cpu core. over a third less CPU. This is with the same 128k hash table size.

CPU usage calculated by taking the time pf purge ran and dividing it by the difference between now, and lstart:

ps -ax -o lstart,time,command | grep pf; date

Sun Sep 9 16:31:23 2018 0:30.95 [pf purge]
Sun Sep 9 19:33:53 2018 0:00.02 grep pf
Sun Sep 9 19:33:53 UTC 2018

note, if you apply the diff and check it w/ svn diff -x -wb, you'll see that the only change is the comment, the if, and the braces.

All comments are minor.

ok, this should be ready to land.. Not going to update printf(9),
that'll be a different patch, and there are missing vprintf and vlog
from the body of the man page...

add fix for when printf returns an error, pass the error up
correctly.. prf_buf does not return an error..

All but the printf error is addressed, and the next patch will address this issue.

Fix most of the issues. I will address the cast to int w/ a KASSERT in another update.

Update and address various comments.

simple fix, limit local_read_rate to be 0 or 1. Even more simple that the fix that was committed.

@delphij this is my comment copied over from https://reviews.freebsd.org/D16866?id=47165 that was unaddressed.

I'll update the patch shortly. Thanks for the review.

This does add some tests that were missing. I can commit those and the MLINK for _putbuf separately if people would like. I'm also fine breaking out the sys/kern/subr_bus.c changes as well.

Update. Use a new created printf drain function that works in both
userland and kernel. Document the function, and add tests for it.

Please commit these patches separately. They deal w/ different issues.

IMO, looks good to me.

Working on testing sbuf, and adding new drain function from printing, w/ both a userland and kernel versions of the function.

Thanks for the comments, you solved some of the issues I knew about and didn't know how to solve best.

Should powerpc64 be added too?

looks fine. Agree that conditioned output should be used. We do our own conditioning so it wouldn't be a major problem to use the raw, but the raw will likely have less entropy.

looks good. Thanks.

I cannot comment on this as __pure2 is not documented, so I don't know the expected semantics of adding that define.

no comments...

Oh, note that the above are my opinions, and they differ significantly from most of the rest of the FreeBSD team. Mainly why I haven't spent much time working on the rng system,

In D15526#330717, @mmacy wrote:

Collecting the first 10 packets or so every second is not a great idea. With things like tcp off-load which bursts packets, it's common for bursts of packets to be correlated.

Yes. True. Nonetheless, I'm much more interested in the questions I raised in the review. All of these implementation details are secondary to those.

In D15526#330707, @mmacy wrote:

While certainly better than what we were doing, global pps accounting would not be efficient multi-core. I'd much rather have it collect the first 10 packets or so every second.

In D15526#330054, @gallatin wrote:

If people want to use ethernet entropy harvesting, can we do something to make it more useful in a separate change? I was going to initially suggest the ether dst addr, but that's not very random either.

In any cases, I think these changes are a huge win. Especially moving the collection mask outside a cacheline that is written in the hot path.

In D15446#330696, @sef wrote:

Yes, we have done this before. When I did the GCM work under contract for the FreeBSD Foundation, they paid for a third party reviewer to go over the code and make sure it was correct. So, yes, we have done this before.

Oh, so *I* don't have to go find one and pay for it. That was not clear. Phew.

In D15446#329285, @sef wrote:

I was unable to review that the code matches an implementation, as the code does not state what implementation it implements. Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

Then there is no point to responding to anything in this, since I cannot afford to hire a cryptographer, and all my attempts to get someone with actual crypto experience to work on it were ignored or responded with "it's easy, you can just see what the aes-gcm code does."

In D15446#329550, @mav wrote:

In D15446#329261, @jmg wrote:

Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

That's something new to me. Did we ever do that before? Who and how shall do that? I can understand when secteam@ review is requested for crypto code, that is reasonable, but who is a "professional cryptographer"? I think it was kind of agreed before that every review in this area usually starts from "I am not a cryptographer, but ..." disclaimer.

Per comments, there are a number of improvements that should be made.

I agree w/ cem that we need a general solution to this. I'm fine w/ this patch, but I do question the wisdom in making this code aesni only. This code really needs to be turned into a library which any OCF driver can use, as this performance problem is not limited to aesni.

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...