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
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
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:
- source is disabled by default
- there are calls only in tmpfs and ufs, where the former has a copy pasted call and the comment from ufs
- 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.