On Fri, Nov 15, 2024 at 08:03:00PM +1100, NeilBrown wrote: > 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. That's a good RAM size for this test, but I think you do need to have more than a single client to stress the server a little more. > 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. Ah, that is sensible. A comment might be added that explains that the server response to the client's "request" to increase the slot table size does not have to be immediate, and in fact, the failure is a sign the server is under memory pressure and shouldn't grow the slot table size anyway. > 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. I don't think it will ever fail if the requested allocation is below 4KB, so this might be no more than a fussy detail. It does indeed make sense to me, though. > 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. -- Chuck Lever
