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 - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 12356

Event Timeline

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

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.

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

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

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

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.

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?

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.