>> /*
>> - * The array exist, but the particular memcg does not. That is an
>> - * impossible situation: it would mean we are trying to add to a list
>> - * belonging to a memcg that does not exist. Either wasn't created or
>> - * has been already freed. In both cases it should no longer have
>> - * objects. BUG_ON to avoid a NULL dereference.
>> + * The array exist, but the particular memcg does not. This cannot
>> + * happen when we are called from memcg_kmem_lru_of_page with a
>> + * definite memcg, but it can happen when we are iterating over all
>> + * memcgs (for instance, when disposing all lists.
>> */
>> - BUG_ON(!lru->memcg_lrus[index]);
>> + if (!lru->memcg_lrus[index]) {
>> + rcu_read_unlock();
>> + return NULL;
>> + }
>
> It took 28 patches, but my head is now spinning and my vision is fading
> in and out.
>
You're a hero.
>> nlru = &lru->memcg_lrus[index]->node[nid];
>> rcu_read_unlock();
>> return nlru;
>> @@ -80,6 +83,23 @@ memcg_kmem_lru_of_page(struct list_lru *lru, struct page *page)
>> return lru_node_of_index(lru, memcg_id, nid);
>> }
>>
>> +/*
>> + * This helper will loop through all node-data in the LRU, either global or
>> + * per-memcg. If memcg is either not present or not used,
>> + * memcg_limited_groups_array_size will be 0. _idx starts at -1, and it will
>> + * still be allowed to execute once.
>> + *
>> + * We convention that for _idx = -1, the global node info should be used.
>
> I don't think that "convention" is a verb, but I rather like the way
> it is used here.
>
We can convention to do it this way from now on.
>> + * After that, we will go through each of the memcgs, starting at 0.
>> + *
>> + * We don't need any kind of locking for the loop because
>> + * memcg_limited_groups_array_size can only grow, gaining new fields at the
>> + * end. The old ones are just copied, and any interesting manipulation happen
>> + * in the node list itself, and we already lock the list.
>
> Might be worth mentioning what type _idx should have. Although I suspect
> the code will work OK if _idx has unsigned type.
>
We convention -1 to be "no memcg", so it has to be an int.
>> + */
>> +#define for_each_memcg_lru_index(_idx) \
>> + for ((_idx) = -1; ((_idx) < memcg_limited_groups_array_size); (_idx)++)
>> +
>> int
>> list_lru_add(
>> struct list_lru *lru,
>> @@ -139,10 +159,19 @@ list_lru_del(
>> EXPORT_SYMBOL_GPL(list_lru_del);
>>
>>
>> unsigned long
>> -list_lru_walk_node(
>> +list_lru_walk_node_memcg(
>> struct list_lru *lru,
>> int nid,
>> list_lru_walk_cb isolate,
>> void *cb_arg,
>> - unsigned long *nr_to_walk)
>> + unsigned long *nr_to_walk,
>> + struct mem_cgroup *memcg)
>> {
>> - struct list_lru_node *nlru = &lru->node[nid];
>> struct list_head *item, *n;
>> unsigned long isolated = 0;
>> + struct list_lru_node *nlru;
>> + int memcg_id = -1;
>> +
>> + if (memcg && memcg_kmem_is_active(memcg))
>> + memcg_id = memcg_cache_id(memcg);
>
> Could use a helper function for this I guess. The nice thing about
> this is that it gives one a logical place at which to describe what's
> going on.
>
Ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
