On 03.10.2022 17:27, Michal Hocko wrote: > On Mon 03-10-22 17:09:15, Alexander Fedorov wrote: >> On 03.10.2022 16:32, Michal Hocko wrote: >>> On Mon 03-10-22 15:47:10, Alexander Fedorov wrote: >>>> @@ -3197,17 +3197,30 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) >>>> stock->nr_bytes = 0; >>>> } >>>> >>>> - obj_cgroup_put(old); >>>> + /* >>>> + * Clear pointer before freeing memory so that >>>> + * drain_all_stock() -> obj_stock_flush_required() >>>> + * does not see a freed pointer. >>>> + */ >>>> stock->cached_objcg = NULL; >>>> + obj_cgroup_put(old); >>> >>> Do we need barrier() or something else to ensure there is no reordering? >>> I am not reallyu sure what kind of barriers are implied by the pcp ref >>> counting. >> >> obj_cgroup_put() -> kfree_rcu() -> synchronize_rcu() should take care >> of this: > > This is a very subtle guarantee. Also it would only apply if this is the > last reference, right? Hmm, yes, for the last reference only, also not sure about pcp ref counter ordering rules for previous references. > Is there any reason to not use > WRITE_ONCE(stock->cached_objcg, NULL); > obj_cgroup_put(old); > > IIRC this should prevent any reordering. Now that I think about it we actually must use WRITE_ONCE everywhere when writing cached_objcg because otherwise compiler might split the pointer-sized store into several smaller-sized ones (store tearing), and obj_stock_flush_required() would read garbage instead of pointer. And thinking about memory barriers, maybe we need them too alongside WRITE_ONCE when setting pointer to non-null value? Otherwise drain_all_stock() -> obj_stock_flush_required() might read old data. Since that's exactly what rcu_assign_pointer() does, it seems that we are going back to using rcu_*() primitives everywhere?
