On Thu, 14 Nov 2024, Chuck Lever III wrote: > > > > On Nov 13, 2024, at 12:38 AM, NeilBrown <neilb@xxxxxxx> wrote: > > > > This patch set aims to allocate session-based DRC slots on demand, and > > free them when not in use, or when memory is tight. > > > > I've tested with NFSD_MAX_UNUSED_SLOTS set to 1 so that freeing is > > overly agreesive, and with lots of printks, and it seems to do the right > > thing, though memory pressure has never freed anything - I think you > > need several clients with a non-trivial number of slots allocated before > > the thresholds in the shrinker code will trigger any freeing. > > Can you describe your test set-up? Generally a system > with less than 4GB of memory can trigger shrinkers > pretty easily. > > If we never see the mechanism being triggered due to > memory exhaustion, then I wonder if the additional > complexity is adding substantial value. Just a single VM with 1G RAM. Only one client so only one session. The default batch count for shrinkers is 64 and the reported count of freeable items is normally scaled down a lot until memory gets really tight. So if I only have 6 slots that could be freed the shrinker isn't going to notice. I set ->batch to 2 and ->seeks to 0 and the shrinker started freeing things. This allowed me to see some bugs. One that I haven't resolved yet is the need to wait to get confirmation from the client before rejecting requests with larger numbered slots. > > > > I haven't made use of the CB_RECALL_SLOT callback. I'm not sure how > > useful that is. There are certainly cases where simply setting the > > target in a SEQUENCE reply might not be enough, but I doubt they are > > very common. You would need a session to be completely idle, with the > > last request received on it indicating that lots of slots were still in > > use. > > > > Currently we allocate slots one at a time when the last available slot > > was used by the client, and only if a NOWAIT allocation can succeed. It > > is possible that this isn't quite agreesive enough. When performing a > > lot of writeback it can be useful to have lots of slots, but memory > > pressure is also likely to build up on the server so GFP_NOWAIT is likely > > to fail. Maybe occasionally using a firmer request (outside the > > spinlock) would be justified. > > I'm wondering why GFP_NOWAIT is used here, and I admit > I'm not strongly familiar with the code or mechanism. > Why not always use GFP_KERNEL ? Partly because the kmalloc call is under a spinlock, so we cannot wait. But that could be changed with a bit of work. GFP_KERNEL can block indefinitely, and we don't actually need the allocation to succeed to satisfy the current request, so it seems wrong to block at all when we don't need to. I'm hoping that GFP_NOWAIT will succeed often enough that the slot table will grow when there is demand - maybe not instantly but not too slowly. If GFP_NOWAIT doesn't succeed, then reclaim will be happening and the shrinker will probably ask us to return some slots soon - maybe it isn't worth trying hard to allocate something we will have to return soon. > > > > We free slots when the number of unused slots passes some threshold - > > currently 6 (because ... why not). Possible a hysteresis should be > > added so we don't free unused slots for a least N seconds. > > Generally freeing unused resources is un-Linux like. :-) > Can you provide a rationale for why this is needed? Uhm... No. I added it so that patch which adds slot retirement could do something useful before the shrinker was added, and when I added the shrinker I couldn't bring myself to remove it. Probably I should. Thanks for your thoughtful review. NeilBrown > > > > When the shrinker wants to apply presure we remove slots equally from > > all sessions. Maybe there should be some proportionality but that would > > be more complex and I'm not sure it would gain much. Slot 0 can never > > be freed of course. > > > > I'm very interested to see what people think of the over-all approach, > > and of the specifics of the code. > > > > Thanks, > > NeilBrown > > > > > > [PATCH 1/4] nfsd: remove artificial limits on the session-based DRC > > [PATCH 2/4] nfsd: allocate new session-based DRC slots on demand. > > [PATCH 3/4] nfsd: free unused session-DRC slots > > [PATCH 4/4] nfsd: add shrinker to reduce number of slots allocated > > > > -- > Chuck Lever > > >
