Page MenuHomeFreeBSD

Remove entropy harvesting from tmpfs atime
Needs ReviewPublic

Authored by mjg on Nov 1 2017, 8:20 AM.

Details

Reviewers
jmg
Group Reviewers
secteam
Summary

The current harvesting code is extremely smp-unfriendly and I don't see any value of having this added here. I could not be bothered to benchmark this particular change in isolation - there are several unrelated global problems which also need cleaning up. I don't feel strongly about it, but without a good reason to keep it I would strongly prefer to see it go.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 12356

Event Timeline

mjg created this revision.Nov 1 2017, 8:20 AM
emaste added a subscriber: emaste.Dec 19 2017, 2:20 PM

This looks reasonable to me.
@jmg, your thoughts?

jmg added a comment.Dec 23 2017, 1:53 AM

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;

Which is before the lock. IMO, FS_ATIME should just go entirely, not just from tmp fs, but I don't see how this code could be a problem.

mjg added a comment.Dec 23 2017, 4:00 AM

on kernels i tested harvest_context happens to be aligned at cache size. then hc_source_mask occupies the same line as beginning of the hc_entropy_ring:

struct entropy_ring {
        struct harvest_event ring[RANDOM_RING_MAX];
        volatile u_int in;
        volatile u_int out;
} hc_entropy_ring;

i.e. this induces avoidable fetches

the mask can and should be moved out and annotated with __read_mostly/frequently, but the harvesting in here should be whacked regardless

mmacy added a subscriber: mmacy.May 22 2018, 4:42 AM

Even at barely 3Mpps ether_input spends 30% of its time in random_harvest:

https://people.freebsd.org/~mmacy/2018.05.11/udprx1.svg

mjg added a comment.Aug 21 2019, 5:37 AM

Can someone from secteam take a stance on this?

The scalability problem mentioned previously was taken care of, but the doubts about usefulness of this code remain.

Note the that:

  1. source is disabled by default
  2. there are calls only in tmpfs and ufs, where the former has a copy pasted call and the comment from ufs
  3. given the above there is no equivalent call in zfs

If this feature is of any use (which I doubt) it should probably get placed in ag eneral way and removed from fs-specific code (alternatively zfs can be updated to also have it). The current state does not make sense.

Also note just removing the option is a little problematic in that the mask is configured precisely as a bitmask as opposed to a keyword list or anything else extendable.

cem added a subscriber: cem.EditedAug 21 2019, 7:50 AM

Is this still a material perf problem with extracted hc_source_mask (__read_mostly and checked inline in the header), and the bit disabled by default? Or is the goal at this point more aesthetic?

mjg added a comment.Aug 22 2019, 12:05 AM

I noted there is no perf issue anymore. Now the code simply does not make sense to be only found in tmpfs + ufs and not in (now) flagship fs (zfs). It should be in all, in general code, or preferably removed.