Chris Li <chrisl@xxxxxxxxxx> 于2023年11月23日周四 04:57写道： > > Hi Kairui, > > On Wed, Nov 22, 2023 at 9:33 AM Kairui Song <ryncsn@xxxxxxxxx> wrote: > > > > There are two different charges. Memcg charging and memcg swapin charging. > > > The folio_put will do the memcg discharge, the corresponding memcg > > > charge is in follio allocation. > > > > Hi Chris, > > > > I didn't get your idea here... By "memcg swapin charge", do you mean > > "memory.swap.*"? And "memcg charging" means "memory.*"?. There is no > > Sorry I should have used the function name then there is no ambiguity. > "memcg swapin charge" I mean function mem_cgroup_swapin_charge_folio(). > This function will look up the swap entry and find the memcg by swap entry then > charge to that memcg. > > > memcg charge related code in folio allocation (alloc_pages_mpol), > > actually the mem_cgroup_swapin_charge_folio here is doing memcg charge > > not memcg swapin charge. Swapin path actually need to uncharge > > "memory.swap" by mem_cgroup_swapin_uncharge_swap in later part of this > > function. > > I still think you have a bug there. > > Take this make up example: > Let say the for loop runs 3 times and the 3rd time breaks out the for loop. > The original code will call: > filemap_get_folio() 3 times > folio_put() 2 times > mem_cgroup_swapin_charge_folio() 1 time. > > With your patch, it will call: > filemap_get_folio() 3 times > folio_put() 2 times > mem_cgroup_swapin_charge_folio() 3 times. > > Do you see the behavior difference there? Hi Chris. folio_put will discharge a page if it's charged, in original code the 2 folio_put call simply free the page since it's not charged. But in this patch, folio_put will cancel previous mem_cgroup_swapin_charge_folio call, so actually the 3 mem_cgroup_swapin_charge_folio calls will only charge once. (2 calls was cancelled by folio_put). I think this is making it confusing indeed and causing more trouble in error path (the uncharge could be more expensive than unlock check), will rework this part.
