Messages in this thread | | | From | Waiman Long <> | Subject | Re: [PATCH v3 5/5] mm/memcg: Optimize user context object stock access | Date | Thu, 15 Apr 2021 15:06:28 -0400 |
| |
On 4/15/21 2:53 PM, Johannes Weiner wrote: > On Thu, Apr 15, 2021 at 02:16:17PM -0400, Waiman Long wrote: >> On 4/15/21 1:53 PM, Johannes Weiner wrote: >>> On Tue, Apr 13, 2021 at 09:20:27PM -0400, Waiman Long wrote: >>>> Most kmem_cache_alloc() calls are from user context. With instrumentation >>>> enabled, the measured amount of kmem_cache_alloc() calls from non-task >>>> context was about 0.01% of the total. >>>> >>>> The irq disable/enable sequence used in this case to access content >>>> from object stock is slow. To optimize for user context access, there >>>> are now two object stocks for task context and interrupt context access >>>> respectively. >>>> >>>> The task context object stock can be accessed after disabling preemption >>>> which is cheap in non-preempt kernel. The interrupt context object stock >>>> can only be accessed after disabling interrupt. User context code can >>>> access interrupt object stock, but not vice versa. >>>> >>>> The mod_objcg_state() function is also modified to make sure that memcg >>>> and lruvec stat updates are done with interrupted disabled. >>>> >>>> The downside of this change is that there are more data stored in local >>>> object stocks and not reflected in the charge counter and the vmstat >>>> arrays. However, this is a small price to pay for better performance. >>>> >>>> Signed-off-by: Waiman Long <longman@redhat.com> >>>> Acked-by: Roman Gushchin <guro@fb.com> >>>> Reviewed-by: Shakeel Butt <shakeelb@google.com> >>> This makes sense, and also explains the previous patch a bit >>> better. But please merge those two. >> The reason I broke it into two is so that the patches are individually >> easier to review. I prefer to update the commit log of patch 4 to explain >> why the obj_stock structure is introduced instead of merging the two. > Well I did not find them easier to review separately. > >>>> @@ -2327,7 +2365,9 @@ static void drain_local_stock(struct work_struct *dummy) >>>> local_irq_save(flags); >>>> stock = this_cpu_ptr(&memcg_stock); >>>> - drain_obj_stock(&stock->obj); >>>> + drain_obj_stock(&stock->irq_obj); >>>> + if (in_task()) >>>> + drain_obj_stock(&stock->task_obj); >>>> drain_stock(stock); >>>> clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags); >>>> @@ -3183,7 +3223,7 @@ static inline void mod_objcg_state(struct obj_cgroup *objcg, >>>> memcg = obj_cgroup_memcg(objcg); >>>> if (pgdat) >>>> lruvec = mem_cgroup_lruvec(memcg, pgdat); >>>> - __mod_memcg_lruvec_state(memcg, lruvec, idx, nr); >>>> + mod_memcg_lruvec_state(memcg, lruvec, idx, nr); >>>> rcu_read_unlock(); >>> This is actually a bug introduced in the earlier patch, isn't it? >>> Calling __mod_memcg_lruvec_state() without irqs disabled... >>> >> Not really, in patch 3, mod_objcg_state() is called only in the stock update >> context where interrupt had already been disabled. But now, that is no >> longer the case, that is why i need to update mod_objcg_state() to make sure >> irq is disabled before updating vmstat data array. > Oh, I see it now. Man, that's subtle. We've had several very hard to > track down preemption bugs in those stats, because they manifest as > counter imbalances and you have no idea if there is a leak somewhere. > > The convention for these functions is that the __ prefix indicates > that preemption has been suitably disabled. Please always follow this > convention, even if the semantic change is temporary. I see. I will fix that in the next version. > > Btw, is there a reason why the stock caching isn't just part of > mod_objcg_state()? Why does the user need to choose if they want the > caching or not? It's not like we ask for this when charging, either. > Yes, I can revert it to make mod_objcg_state() to call mod_obj_stock_state() internally instead of the other way around.
Cheers, Longman
| |