| Message ID | 20210419000032.5432-6-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memcg: Reduce kmemcache memory accounting overhead | expand |
On Mon, Apr 19, 2021 at 8:01 AM Waiman Long <longman@redhat.com> wrote: > > There are two issues with the current refill_obj_stock() code. First of > all, when nr_bytes reaches over PAGE_SIZE, it calls drain_obj_stock() to > atomically flush out remaining bytes to obj_cgroup, clear cached_objcg > and do a obj_cgroup_put(). It is likely that the same obj_cgroup will > be used again which leads to another call to drain_obj_stock() and > obj_cgroup_get() as well as atomically retrieve the available byte from > obj_cgroup. That is costly. Instead, we should just uncharge the excess > pages, reduce the stock bytes and be done with it. The drain_obj_stock() > function should only be called when obj_cgroup changes. > > Secondly, when charging an object of size not less than a page in > obj_cgroup_charge(), it is possible that the remaining bytes to be > refilled to the stock will overflow a page and cause refill_obj_stock() > to uncharge 1 page. To avoid the additional uncharge in this case, > a new overfill flag is added to refill_obj_stock() which will be set > when called from obj_cgroup_charge(). > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > mm/memcontrol.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index a6dd18f6d8a8..d13961352eef 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3357,23 +3357,34 @@ static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, > return false; > } > > -static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) > +static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, > + bool overfill) > { > unsigned long flags; > struct obj_stock *stock = get_obj_stock(&flags); > + unsigned int nr_pages = 0; > > if (stock->cached_objcg != objcg) { /* reset if necessary */ > - drain_obj_stock(stock); > + if (stock->cached_objcg) > + drain_obj_stock(stock); > obj_cgroup_get(objcg); > stock->cached_objcg = objcg; > stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); > } > stock->nr_bytes += nr_bytes; > > - if (stock->nr_bytes > PAGE_SIZE) > - drain_obj_stock(stock); > + if (!overfill && (stock->nr_bytes > PAGE_SIZE)) { > + nr_pages = stock->nr_bytes >> PAGE_SHIFT; > + stock->nr_bytes &= (PAGE_SIZE - 1); > + } > > put_obj_stock(flags); > + > + if (nr_pages) { > + rcu_read_lock(); > + __memcg_kmem_uncharge(obj_cgroup_memcg(objcg), nr_pages); > + rcu_read_unlock(); > + } It is not safe to call __memcg_kmem_uncharge() under rcu lock and without holding a reference to memcg. More details can refer to the following link. https://lore.kernel.org/linux-mm/20210319163821.20704-2-songmuchun@bytedance.com/ In the above patchset, we introduce obj_cgroup_uncharge_pages to uncharge some pages from object cgroup. You can use this safe API. Thanks. > } > > int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) > @@ -3410,7 +3421,7 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) > > ret = __memcg_kmem_charge(memcg, gfp, nr_pages); > if (!ret && nr_bytes) > - refill_obj_stock(objcg, PAGE_SIZE - nr_bytes); > + refill_obj_stock(objcg, PAGE_SIZE - nr_bytes, true); > > css_put(&memcg->css); > return ret; > @@ -3418,7 +3429,7 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) > > void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size) > { > - refill_obj_stock(objcg, size); > + refill_obj_stock(objcg, size, false); > } > > #endif /* CONFIG_MEMCG_KMEM */ > -- > 2.18.1 >
On Sun, Apr 18, 2021 at 11:07 PM Muchun Song <songmuchun@bytedance.com> wrote: > > On Mon, Apr 19, 2021 at 8:01 AM Waiman Long <longman@redhat.com> wrote: > > > > There are two issues with the current refill_obj_stock() code. First of > > all, when nr_bytes reaches over PAGE_SIZE, it calls drain_obj_stock() to > > atomically flush out remaining bytes to obj_cgroup, clear cached_objcg > > and do a obj_cgroup_put(). It is likely that the same obj_cgroup will > > be used again which leads to another call to drain_obj_stock() and > > obj_cgroup_get() as well as atomically retrieve the available byte from > > obj_cgroup. That is costly. Instead, we should just uncharge the excess > > pages, reduce the stock bytes and be done with it. The drain_obj_stock() > > function should only be called when obj_cgroup changes. > > > > Secondly, when charging an object of size not less than a page in > > obj_cgroup_charge(), it is possible that the remaining bytes to be > > refilled to the stock will overflow a page and cause refill_obj_stock() > > to uncharge 1 page. To avoid the additional uncharge in this case, > > a new overfill flag is added to refill_obj_stock() which will be set > > when called from obj_cgroup_charge(). > > > > Signed-off-by: Waiman Long <longman@redhat.com> > > --- > > mm/memcontrol.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index a6dd18f6d8a8..d13961352eef 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -3357,23 +3357,34 @@ static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, > > return false; > > } > > > > -static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) > > +static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, > > + bool overfill) > > { > > unsigned long flags; > > struct obj_stock *stock = get_obj_stock(&flags); > > + unsigned int nr_pages = 0; > > > > if (stock->cached_objcg != objcg) { /* reset if necessary */ > > - drain_obj_stock(stock); > > + if (stock->cached_objcg) > > + drain_obj_stock(stock); > > obj_cgroup_get(objcg); > > stock->cached_objcg = objcg; > > stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); > > } > > stock->nr_bytes += nr_bytes; > > > > - if (stock->nr_bytes > PAGE_SIZE) > > - drain_obj_stock(stock); > > + if (!overfill && (stock->nr_bytes > PAGE_SIZE)) { > > + nr_pages = stock->nr_bytes >> PAGE_SHIFT; > > + stock->nr_bytes &= (PAGE_SIZE - 1); > > + } > > > > put_obj_stock(flags); > > + > > + if (nr_pages) { > > + rcu_read_lock(); > > + __memcg_kmem_uncharge(obj_cgroup_memcg(objcg), nr_pages); > > + rcu_read_unlock(); > > + } > > It is not safe to call __memcg_kmem_uncharge() under rcu lock > and without holding a reference to memcg. More details can refer > to the following link. > > https://lore.kernel.org/linux-mm/20210319163821.20704-2-songmuchun@bytedance.com/ > > In the above patchset, we introduce obj_cgroup_uncharge_pages to > uncharge some pages from object cgroup. You can use this safe > API. > I would recommend just rebase the patch series over the latest mm tree.
On 4/19/21 11:00 AM, Shakeel Butt wrote: > On Sun, Apr 18, 2021 at 11:07 PM Muchun Song <songmuchun@bytedance.com> wrote: >> On Mon, Apr 19, 2021 at 8:01 AM Waiman Long <longman@redhat.com> wrote: >>> There are two issues with the current refill_obj_stock() code. First of >>> all, when nr_bytes reaches over PAGE_SIZE, it calls drain_obj_stock() to >>> atomically flush out remaining bytes to obj_cgroup, clear cached_objcg >>> and do a obj_cgroup_put(). It is likely that the same obj_cgroup will >>> be used again which leads to another call to drain_obj_stock() and >>> obj_cgroup_get() as well as atomically retrieve the available byte from >>> obj_cgroup. That is costly. Instead, we should just uncharge the excess >>> pages, reduce the stock bytes and be done with it. The drain_obj_stock() >>> function should only be called when obj_cgroup changes. >>> >>> Secondly, when charging an object of size not less than a page in >>> obj_cgroup_charge(), it is possible that the remaining bytes to be >>> refilled to the stock will overflow a page and cause refill_obj_stock() >>> to uncharge 1 page. To avoid the additional uncharge in this case, >>> a new overfill flag is added to refill_obj_stock() which will be set >>> when called from obj_cgroup_charge(). >>> >>> Signed-off-by: Waiman Long <longman@redhat.com> >>> --- >>> mm/memcontrol.c | 23 +++++++++++++++++------ >>> 1 file changed, 17 insertions(+), 6 deletions(-) >>> >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>> index a6dd18f6d8a8..d13961352eef 100644 >>> --- a/mm/memcontrol.c >>> +++ b/mm/memcontrol.c >>> @@ -3357,23 +3357,34 @@ static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, >>> return false; >>> } >>> >>> -static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) >>> +static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, >>> + bool overfill) >>> { >>> unsigned long flags; >>> struct obj_stock *stock = get_obj_stock(&flags); >>> + unsigned int nr_pages = 0; >>> >>> if (stock->cached_objcg != objcg) { /* reset if necessary */ >>> - drain_obj_stock(stock); >>> + if (stock->cached_objcg) >>> + drain_obj_stock(stock); >>> obj_cgroup_get(objcg); >>> stock->cached_objcg = objcg; >>> stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); >>> } >>> stock->nr_bytes += nr_bytes; >>> >>> - if (stock->nr_bytes > PAGE_SIZE) >>> - drain_obj_stock(stock); >>> + if (!overfill && (stock->nr_bytes > PAGE_SIZE)) { >>> + nr_pages = stock->nr_bytes >> PAGE_SHIFT; >>> + stock->nr_bytes &= (PAGE_SIZE - 1); >>> + } >>> >>> put_obj_stock(flags); >>> + >>> + if (nr_pages) { >>> + rcu_read_lock(); >>> + __memcg_kmem_uncharge(obj_cgroup_memcg(objcg), nr_pages); >>> + rcu_read_unlock(); >>> + } >> It is not safe to call __memcg_kmem_uncharge() under rcu lock >> and without holding a reference to memcg. More details can refer >> to the following link. >> >> https://lore.kernel.org/linux-mm/20210319163821.20704-2-songmuchun@bytedance.com/ >> >> In the above patchset, we introduce obj_cgroup_uncharge_pages to >> uncharge some pages from object cgroup. You can use this safe >> API. >> > I would recommend just rebase the patch series over the latest mm tree. > I see, I will rebase it to the latest mm tree. Thanks, Longman
On 4/19/21 2:06 AM, Muchun Song wrote: > On Mon, Apr 19, 2021 at 8:01 AM Waiman Long <longman@redhat.com> wrote: >> There are two issues with the current refill_obj_stock() code. First of >> all, when nr_bytes reaches over PAGE_SIZE, it calls drain_obj_stock() to >> atomically flush out remaining bytes to obj_cgroup, clear cached_objcg >> and do a obj_cgroup_put(). It is likely that the same obj_cgroup will >> be used again which leads to another call to drain_obj_stock() and >> obj_cgroup_get() as well as atomically retrieve the available byte from >> obj_cgroup. That is costly. Instead, we should just uncharge the excess >> pages, reduce the stock bytes and be done with it. The drain_obj_stock() >> function should only be called when obj_cgroup changes. >> >> Secondly, when charging an object of size not less than a page in >> obj_cgroup_charge(), it is possible that the remaining bytes to be >> refilled to the stock will overflow a page and cause refill_obj_stock() >> to uncharge 1 page. To avoid the additional uncharge in this case, >> a new overfill flag is added to refill_obj_stock() which will be set >> when called from obj_cgroup_charge(). >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> mm/memcontrol.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index a6dd18f6d8a8..d13961352eef 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -3357,23 +3357,34 @@ static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, >> return false; >> } >> >> -static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) >> +static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, >> + bool overfill) >> { >> unsigned long flags; >> struct obj_stock *stock = get_obj_stock(&flags); >> + unsigned int nr_pages = 0; >> >> if (stock->cached_objcg != objcg) { /* reset if necessary */ >> - drain_obj_stock(stock); >> + if (stock->cached_objcg) >> + drain_obj_stock(stock); >> obj_cgroup_get(objcg); >> stock->cached_objcg = objcg; >> stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); >> } >> stock->nr_bytes += nr_bytes; >> >> - if (stock->nr_bytes > PAGE_SIZE) >> - drain_obj_stock(stock); >> + if (!overfill && (stock->nr_bytes > PAGE_SIZE)) { >> + nr_pages = stock->nr_bytes >> PAGE_SHIFT; >> + stock->nr_bytes &= (PAGE_SIZE - 1); >> + } >> >> put_obj_stock(flags); >> + >> + if (nr_pages) { >> + rcu_read_lock(); >> + __memcg_kmem_uncharge(obj_cgroup_memcg(objcg), nr_pages); >> + rcu_read_unlock(); >> + } > It is not safe to call __memcg_kmem_uncharge() under rcu lock > and without holding a reference to memcg. More details can refer > to the following link. > > https://lore.kernel.org/linux-mm/20210319163821.20704-2-songmuchun@bytedance.com/ > > In the above patchset, we introduce obj_cgroup_uncharge_pages to > uncharge some pages from object cgroup. You can use this safe > API. Thanks for the comment. Will update my patch with call to obj_cgroup_uncharge_pages(). Cheers, Longman
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a6dd18f6d8a8..d13961352eef 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3357,23 +3357,34 @@ static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, return false; } -static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) +static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, + bool overfill) { unsigned long flags; struct obj_stock *stock = get_obj_stock(&flags); + unsigned int nr_pages = 0; if (stock->cached_objcg != objcg) { /* reset if necessary */ - drain_obj_stock(stock); + if (stock->cached_objcg) + drain_obj_stock(stock); obj_cgroup_get(objcg); stock->cached_objcg = objcg; stock->nr_bytes = atomic_xchg(&objcg->nr_charged_bytes, 0); } stock->nr_bytes += nr_bytes; - if (stock->nr_bytes > PAGE_SIZE) - drain_obj_stock(stock); + if (!overfill && (stock->nr_bytes > PAGE_SIZE)) { + nr_pages = stock->nr_bytes >> PAGE_SHIFT; + stock->nr_bytes &= (PAGE_SIZE - 1); + } put_obj_stock(flags); + + if (nr_pages) { + rcu_read_lock(); + __memcg_kmem_uncharge(obj_cgroup_memcg(objcg), nr_pages); + rcu_read_unlock(); + } } int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) @@ -3410,7 +3421,7 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) ret = __memcg_kmem_charge(memcg, gfp, nr_pages); if (!ret && nr_bytes) - refill_obj_stock(objcg, PAGE_SIZE - nr_bytes); + refill_obj_stock(objcg, PAGE_SIZE - nr_bytes, true); css_put(&memcg->css); return ret; @@ -3418,7 +3429,7 @@ int obj_cgroup_charge(struct obj_cgroup *objcg, gfp_t gfp, size_t size) void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size) { - refill_obj_stock(objcg, size); + refill_obj_stock(objcg, size, false); } #endif /* CONFIG_MEMCG_KMEM */
There are two issues with the current refill_obj_stock() code. First of all, when nr_bytes reaches over PAGE_SIZE, it calls drain_obj_stock() to atomically flush out remaining bytes to obj_cgroup, clear cached_objcg and do a obj_cgroup_put(). It is likely that the same obj_cgroup will be used again which leads to another call to drain_obj_stock() and obj_cgroup_get() as well as atomically retrieve the available byte from obj_cgroup. That is costly. Instead, we should just uncharge the excess pages, reduce the stock bytes and be done with it. The drain_obj_stock() function should only be called when obj_cgroup changes. Secondly, when charging an object of size not less than a page in obj_cgroup_charge(), it is possible that the remaining bytes to be refilled to the stock will overflow a page and cause refill_obj_stock() to uncharge 1 page. To avoid the additional uncharge in this case, a new overfill flag is added to refill_obj_stock() which will be set when called from obj_cgroup_charge(). Signed-off-by: Waiman Long <longman@redhat.com> --- mm/memcontrol.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)