commited in https://reviews.freebsd.org/rS363683

In D25874#573062, @zeising wrote:

Removed manual pages need to be added to ObsoleteFiles.inc as well.

didn't complain about missing file on command line, that being
rp's module dir..

forgot to remove from files..

too smart w/ command line, include the rest of the files..

I'll wait to get a few more reports of tests before committing..

ok, these should address all of your comments.

update to address hselasky's comments.

make it relative to HEAD.. Post white space commit..

I'm going to commit the change (which the grammar fix) in the next day or two if I don't get any more feedback.

In D17541#569015, @freqlabs wrote:

Yes a PR clarifying this in OpenZFS would be greatly appreciated. Any change here would only be good for MFC. I think the wording suggested by @delphij is a bit more clear.

@freqlabs let ms know if you just want to integrate this into the OpenZFS update, or what.

Looks fine. You should look at unrolling the loop to 3 or 4 rounds. Looking at the A72 optimization guide, it shows that there is a 3 cycle latency, but throughput of 1. Section 4.10 gives example showing three pairs to achieve max perf.

Looks good to me. I'm adding ngie as they did the port to Python 3. I have not run and verified that this works under Python 3, but fully support the move to 3.

In D21886#554614, @imp wrote:

Cool script. It would be better to add the alias with devfs. Then it would disappear w/o devd needing all the info...

I took Daniel O'Connor's script from: https://www.mail-archive.com/freebsd-usb@freebsd.org/msg14258.html

a later diff has addressed these comments.

these should not have been marked done. Why phab did this I have no clue. Maybe they think that if you update a patch you address all of your comments, but that is a TERRIBLE assumption to make.

update with comments from manu

add the path to the diff...

If you want to test, you generate a self sign cert:
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -days 365

In D24945#549225, @gordon wrote:

In D24945#549197, @cem wrote:

We should also disable SSL2, if we do not already. And perhaps TLS 1.0?

SSLv2 already doesn't exist, so no problem there.

TLSv1.0 is still widely deployed. I think removing support entirely is premature.

Tested and working.

Sorry, got a bit excited that things worked.

This booted on my 07S board w/o me having to set hw.ncpu in loader!

In D14429#548455, @skibo wrote:

In D14429#548354, @jmg wrote:

I just tested this on my Cora Z7-07S board, and it looks like there's a hardware bug that prevents this patch from working.

I added a printf to dump mp_maxid and mp_ncpus, and I get 1 and 2 respectively, which per the TRM indicates that it's suppose to have 2 cpus instead of 1.

Crud. I guess the number of CPUs isn't properly represented in the SCU config register. Do you know the value of the entire register? From the u-boot prompt,
type "md 0xf8f00004 1". Another idea is to key off the device field of the SLCR's PSS_IDCODE register at 0xf8000530. Can you get that value too?
It also shows up in a sysctl under "hw.zynq.pss_idcode".

I just tested this on my Cora Z7-07S board, and it looks like there's a hardware bug that prevents this patch from working.

Thanks, I'll look at committing this. I have a Cora Z7 that needs this.

Is there a reason this hasn't been committed?

In D22068#482123, @marcel wrote:

Would you mind splitting the changes into 2 reviews/commits. I don't oppose the changes, but they are logically unrelated/independent and it helps us with reversals and/or MFCs if they are separate commits.

Yeah, after a bit of splunking after writing that comment, I discovered that it was unused.

why was this removed? This was necessary to prevent lock inversion due to a kq being in another kq...

oh, don't forget to bump date!

looks good! Thanks!

Now that memset_s is part of the standard, should we just make explicit_bzero a wrapper for memset_s?

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.