On Mon, Dec 04, 2023 at 04:30:44PM -0800, Chris Li wrote:
On Thu, Nov 30, 2023 at 12:35 PM Johannes Weiner hannes@cmpxchg.org wrote:
On Thu, Nov 30, 2023 at 12:07:41PM -0800, Nhat Pham wrote:
On Thu, Nov 30, 2023 at 11:57 AM Matthew Wilcox willy@infradead.org wrote:
On Thu, Nov 30, 2023 at 11:40:18AM -0800, Nhat Pham wrote:
This patch changes list_lru interface so that the caller must explicitly specify numa node and memcg when adding and removing objects. The old list_lru_add() and list_lru_del() are renamed to list_lru_add_obj() and list_lru_del_obj(), respectively.
Wouldn't it be better to add list_lru_add_memcg() and list_lru_del_memcg() and have:
That is my first thought as well. If we are having two different flavors of LRU add, one has memcg and one without. The list_lru_add() vs list_lru_add_memcg() is the common way to do it.
+bool list_lru_del(struct list_lru *lru, struct list_head *item) +{
int nid = page_to_nid(virt_to_page(item));struct mem_cgroup *memcg = list_lru_memcg_aware(lru) ?mem_cgroup_from_slab_obj(item) : NULL;return list_lru_del_memcg(lru, item, nid, memcg);+}
Seems like _most_ callers will want the original versions and only a few will want the explicit memcg/nid versions. No?
I actually did something along that line in earlier iterations of this patch series (albeit with poorer naming - __list_lru_add() instead of list_lru_add_memcg()). The consensus after some back and forth was that the original list_lru_add() was not a very good design (the better one was this new version that allows for explicit numa/memcg selection). So I agreed to fix it everywhere as a prep patch.
I don't have strong opinions here to be completely honest, but I do think this new API makes more sense (at the cost of quite a bit of elbow grease to fix every callsites and extra reviewing).
Maybe I can shed some light since I was pushing for doing it this way.
The quiet assumption that 'struct list_head *item' is (embedded in) a slab object that is also charged to a cgroup is a bit much, given that nothing in the name or documentation of the function points to that.
We can add it to the document if that is desirable.
It would help, but it still violates the "easy to use, hard to misuse" principle. And I think it does the API layering backwards.
list_lru_add() is the "default" API function. It makes sense to keep that simple and robust, then add add convenience wrappers for additional, specialized functionality like memcg lookups for charged slab objects - even if that's a common usecase.
It's better for a new user to be paused by the require memcg argument in the default function and then go and find list_lru_add_obj(), than it is for somebody to quietly pass an invalid object to list_lru_add() and have subtle runtime problems and crashes (which has happened twice now already).