On 10/23/23 19:00, Christoph Lameter (Ampere) wrote: > On Mon, 23 Oct 2023, Vlastimil Babka wrote: > >>> >>> The slab will be delay frozen when it's picked to actively use by the >>> CPU, it becomes full at the same time, in which case we still need to >>> rely on "frozen" bit to avoid manipulating its list. So the slab will >>> be frozen only when activate use and be unfrozen only when deactivate. >> >> Interesting solution! I wonder if we could go a bit further and remove >> acquire_slab() completely. Because AFAICS even after your changes, >> acquire_slab() is still attempted including freezing the slab, which means >> still doing an cmpxchg_double under the list_lock, and now also handling the >> special case when it failed, but we at least filled percpu partial lists. >> What if we only filled the partial list without freezing, and then froze the >> first slab outside of the list_lock? >> >> Or more precisely, instead of returning the acquired "object" we would >> return the first slab removed from partial list. I think it would simplify >> the code a bit, and further reduce list_lock holding times. >> >> I'll also point out a few more details, but it's not a full detailed review >> as the suggestion above, and another for 4/5, could mean a rather >> significant change for v3. > > This is not that easy. The frozen bit indicates that list management does > not have to be done for a slab if its processed in free. If you take a > slab off the list without setting that bit then something else needs to > provide the information that "frozen" provided. Yes, that's the new slab_node_partial flag in patch 1, protected by list_lock. > If the frozen bit changes can be handled in a different way than > with cmpxchg then that is a good optimization. Frozen bit stays the same, but some scenarios can now avoid it. > For much of the frozen handling we must be holding the node list lock > anyways in order to add/remove from the list. So we already have a lock > that could be used to protect flag operations. I can see the following differences between the traditional frozen bit and the new flag: frozen bit advantage: - __slab_free() on an already-frozen slab can ignore list operations and list_lock completely frozen bit disadvantage: - acquire_slab() trying to do cmpxchg_double() under list_lock (see commit 9b1ea29bc0d7) slab_node_partial flag advantage: - we can take slabs off from node partial list without cmpxchg_double() - probably less cmpxchg_double() operations overall slab_node_partial flag disadvantage: - a __slab_free() that encouters a slab that's not frozen (but slab_node_partial flag is not set) might have to do more work, including taking the list_lock only to find out that slab_node_partial flag is false (but AFAICS that happens only when the slab becomes fully free by the free operation, thus relatively rarely). Put together, I think we might indeed get the best of both if the frozen flag is kept to use for cpu slabs, and we rely on slab_node_partial flag for cpu partial slabs, as the series does.
