Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Vladimir Davydov]

On Tue, Apr 17, 2018 at 09:53:02PM +0300, Kirill Tkhai wrote:
> The patch introduces shrinker::id number, which is used to enumerate
> memcg-aware shrinkers. The number start from 0, and the code tries
> to maintain it as small as possible.
> 
> This will be used as to represent a memcg-aware shrinkers in memcg
> shrinkers map.
> 
> Signed-off-by: Kirill Tkhai 
> ---
>  include/linux/shrinker.h |2 ++
>  mm/vmscan.c  |   51 
> ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index a3894918a436..86b651fa2846 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -66,6 +66,8 @@ struct shrinker {
>  
>   /* These are for internal use */
>   struct list_head list;

> + /* ID in shrinkers_id_idr */
> + int id;

This should be under ifdef CONFIG_MEMCG && CONFIG_SLOB.

>   /* objs pending delete, per node */
>   atomic_long_t *nr_deferred;
>  };
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8b920ce3ae02..4f02fe83537e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -169,6 +169,43 @@ unsigned long vm_total_pages;
>  static LIST_HEAD(shrinker_list);
>  static DECLARE_RWSEM(shrinker_rwsem);
>  
> +#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)

> +static DEFINE_IDR(shrinkers_id_idr);

IMO shrinker_idr would be a better name.

> +
> +static int add_memcg_shrinker(struct shrinker *shrinker)
> +{
> + int id, ret;
> +
> + down_write(_rwsem);
> + ret = id = idr_alloc(_id_idr, shrinker, 0, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto unlock;
> + shrinker->id = id;
> + ret = 0;
> +unlock:
> + up_write(_rwsem);
> + return ret;
> +}
> +
> +static void del_memcg_shrinker(struct shrinker *shrinker)
> +{
> + int id = shrinker->id;
> +
> + down_write(_rwsem);
> + idr_remove(_id_idr, id);
> + up_write(_rwsem);
> +}
> +#else /* CONFIG_MEMCG && !CONFIG_SLOB */
> +static int add_memcg_shrinker(struct shrinker *shrinker)
> +{
> + return 0;
> +}
> +
> +static void del_memcg_shrinker(struct shrinker *shrinker)
> +{
> +}
> +#endif /* CONFIG_MEMCG && !CONFIG_SLOB */
> +
>  #ifdef CONFIG_MEMCG
>  static bool global_reclaim(struct scan_control *sc)
>  {
> @@ -306,6 +343,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum 
> lru_list lru, int zone
>  int register_shrinker(struct shrinker *shrinker)
>  {
>   size_t size = sizeof(*shrinker->nr_deferred);
> + int ret;
>  
>   if (shrinker->flags & SHRINKER_NUMA_AWARE)
>   size *= nr_node_ids;
> @@ -314,10 +352,21 @@ int register_shrinker(struct shrinker *shrinker)
>   if (!shrinker->nr_deferred)
>   return -ENOMEM;
>  
> + if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> + ret = add_memcg_shrinker(shrinker);
> + if (ret)
> + goto free_deferred;
> + }
> +

This doesn't apply anymore, not after commit 8e04944f0ea8a ("mm,vmscan:
Allow preallocating memory for register_shrinker()"). Please rebase.

I guess now you have to allocate an id in prealloc_shrinker and set the
pointer (with idr_replace) in register_shrinker_prepared.

>   down_write(_rwsem);
>   list_add_tail(>list, _list);
>   up_write(_rwsem);
>   return 0;
> +
> +free_deferred:
> + kfree(shrinker->nr_deferred);
> + shrinker->nr_deferred = NULL;
> + return -ENOMEM;
>  }
>  EXPORT_SYMBOL(register_shrinker);
>  
> @@ -328,6 +377,8 @@ void unregister_shrinker(struct shrinker *shrinker)
>  {
>   if (!shrinker->nr_deferred)
>   return;
> + if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> + del_memcg_shrinker(shrinker);
>   down_write(_rwsem);
>   list_del(>list);
>   up_write(_rwsem);
> 

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Vladimir Davydov]

On Tue, Apr 17, 2018 at 09:53:02PM +0300, Kirill Tkhai wrote:
> The patch introduces shrinker::id number, which is used to enumerate
> memcg-aware shrinkers. The number start from 0, and the code tries
> to maintain it as small as possible.
> 
> This will be used as to represent a memcg-aware shrinkers in memcg
> shrinkers map.
> 
> Signed-off-by: Kirill Tkhai 
> ---
>  include/linux/shrinker.h |2 ++
>  mm/vmscan.c  |   51 
> ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index a3894918a436..86b651fa2846 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -66,6 +66,8 @@ struct shrinker {
>  
>   /* These are for internal use */
>   struct list_head list;

> + /* ID in shrinkers_id_idr */
> + int id;

This should be under ifdef CONFIG_MEMCG && CONFIG_SLOB.

>   /* objs pending delete, per node */
>   atomic_long_t *nr_deferred;
>  };
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 8b920ce3ae02..4f02fe83537e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -169,6 +169,43 @@ unsigned long vm_total_pages;
>  static LIST_HEAD(shrinker_list);
>  static DECLARE_RWSEM(shrinker_rwsem);
>  
> +#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)

> +static DEFINE_IDR(shrinkers_id_idr);

IMO shrinker_idr would be a better name.

> +
> +static int add_memcg_shrinker(struct shrinker *shrinker)
> +{
> + int id, ret;
> +
> + down_write(_rwsem);
> + ret = id = idr_alloc(_id_idr, shrinker, 0, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto unlock;
> + shrinker->id = id;
> + ret = 0;
> +unlock:
> + up_write(_rwsem);
> + return ret;
> +}
> +
> +static void del_memcg_shrinker(struct shrinker *shrinker)
> +{
> + int id = shrinker->id;
> +
> + down_write(_rwsem);
> + idr_remove(_id_idr, id);
> + up_write(_rwsem);
> +}
> +#else /* CONFIG_MEMCG && !CONFIG_SLOB */
> +static int add_memcg_shrinker(struct shrinker *shrinker)
> +{
> + return 0;
> +}
> +
> +static void del_memcg_shrinker(struct shrinker *shrinker)
> +{
> +}
> +#endif /* CONFIG_MEMCG && !CONFIG_SLOB */
> +
>  #ifdef CONFIG_MEMCG
>  static bool global_reclaim(struct scan_control *sc)
>  {
> @@ -306,6 +343,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum 
> lru_list lru, int zone
>  int register_shrinker(struct shrinker *shrinker)
>  {
>   size_t size = sizeof(*shrinker->nr_deferred);
> + int ret;
>  
>   if (shrinker->flags & SHRINKER_NUMA_AWARE)
>   size *= nr_node_ids;
> @@ -314,10 +352,21 @@ int register_shrinker(struct shrinker *shrinker)
>   if (!shrinker->nr_deferred)
>   return -ENOMEM;
>  
> + if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> + ret = add_memcg_shrinker(shrinker);
> + if (ret)
> + goto free_deferred;
> + }
> +

This doesn't apply anymore, not after commit 8e04944f0ea8a ("mm,vmscan:
Allow preallocating memory for register_shrinker()"). Please rebase.

I guess now you have to allocate an id in prealloc_shrinker and set the
pointer (with idr_replace) in register_shrinker_prepared.

>   down_write(_rwsem);
>   list_add_tail(>list, _list);
>   up_write(_rwsem);
>   return 0;
> +
> +free_deferred:
> + kfree(shrinker->nr_deferred);
> + shrinker->nr_deferred = NULL;
> + return -ENOMEM;
>  }
>  EXPORT_SYMBOL(register_shrinker);
>  
> @@ -328,6 +377,8 @@ void unregister_shrinker(struct shrinker *shrinker)
>  {
>   if (!shrinker->nr_deferred)
>   return;
> + if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> + del_memcg_shrinker(shrinker);
>   down_write(_rwsem);
>   list_del(>list);
>   up_write(_rwsem);
> 

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

On 18.04.2018 17:32, Tetsuo Handa wrote:
> Kirill Tkhai wrote:
>> On 18.04.2018 17:14, Tetsuo Handa wrote:
>>> Kirill Tkhai wrote:
 The patch introduces shrinker::id number, which is used to enumerate
 memcg-aware shrinkers. The number start from 0, and the code tries
 to maintain it as small as possible.

 This will be used as to represent a memcg-aware shrinkers in memcg
 shrinkers map.
>>>
>>> I'm not reading this thread. But is there reason "id" needs to be managed
>>> using smallest numbers? Can't we use address of shrinker object as "id"
>>> (which will be sparse bitmap, and would be managed using linked list for 
>>> now)?
>>
>> Yes, it's needed to have the smallest numbers, as next patches introduce
>> per-memcg bitmap containing ids of shrinkers.
> 
> If you use sparse bitmap (xbitmap ?), I think you can do it.

There is no implementation in kernel, and search gave me this link:
https://patchwork.kernel.org/patch/10128397/

The problem is that it may allocate memory, and hence to fail.
While adding an element to shrinker lists (and setting a bit
in bitmap) mustn't fail. So, it's not possible to use sparse bitmap.

Kirill

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

On 18.04.2018 17:32, Tetsuo Handa wrote:
> Kirill Tkhai wrote:
>> On 18.04.2018 17:14, Tetsuo Handa wrote:
>>> Kirill Tkhai wrote:
 The patch introduces shrinker::id number, which is used to enumerate
 memcg-aware shrinkers. The number start from 0, and the code tries
 to maintain it as small as possible.

 This will be used as to represent a memcg-aware shrinkers in memcg
 shrinkers map.
>>>
>>> I'm not reading this thread. But is there reason "id" needs to be managed
>>> using smallest numbers? Can't we use address of shrinker object as "id"
>>> (which will be sparse bitmap, and would be managed using linked list for 
>>> now)?
>>
>> Yes, it's needed to have the smallest numbers, as next patches introduce
>> per-memcg bitmap containing ids of shrinkers.
> 
> If you use sparse bitmap (xbitmap ?), I think you can do it.

There is no implementation in kernel, and search gave me this link:
https://patchwork.kernel.org/patch/10128397/

The problem is that it may allocate memory, and hence to fail.
While adding an element to shrinker lists (and setting a bit
in bitmap) mustn't fail. So, it's not possible to use sparse bitmap.

Kirill

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Tetsuo Handa]

Kirill Tkhai wrote:
> On 18.04.2018 17:14, Tetsuo Handa wrote:
> > Kirill Tkhai wrote:
> >> The patch introduces shrinker::id number, which is used to enumerate
> >> memcg-aware shrinkers. The number start from 0, and the code tries
> >> to maintain it as small as possible.
> >>
> >> This will be used as to represent a memcg-aware shrinkers in memcg
> >> shrinkers map.
> > 
> > I'm not reading this thread. But is there reason "id" needs to be managed
> > using smallest numbers? Can't we use address of shrinker object as "id"
> > (which will be sparse bitmap, and would be managed using linked list for 
> > now)?
> 
> Yes, it's needed to have the smallest numbers, as next patches introduce
> per-memcg bitmap containing ids of shrinkers.

If you use sparse bitmap (xbitmap ?), I think you can do it.

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Tetsuo Handa]

Kirill Tkhai wrote:
> On 18.04.2018 17:14, Tetsuo Handa wrote:
> > Kirill Tkhai wrote:
> >> The patch introduces shrinker::id number, which is used to enumerate
> >> memcg-aware shrinkers. The number start from 0, and the code tries
> >> to maintain it as small as possible.
> >>
> >> This will be used as to represent a memcg-aware shrinkers in memcg
> >> shrinkers map.
> > 
> > I'm not reading this thread. But is there reason "id" needs to be managed
> > using smallest numbers? Can't we use address of shrinker object as "id"
> > (which will be sparse bitmap, and would be managed using linked list for 
> > now)?
> 
> Yes, it's needed to have the smallest numbers, as next patches introduce
> per-memcg bitmap containing ids of shrinkers.

If you use sparse bitmap (xbitmap ?), I think you can do it.

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

On 18.04.2018 17:14, Tetsuo Handa wrote:
> Kirill Tkhai wrote:
>> The patch introduces shrinker::id number, which is used to enumerate
>> memcg-aware shrinkers. The number start from 0, and the code tries
>> to maintain it as small as possible.
>>
>> This will be used as to represent a memcg-aware shrinkers in memcg
>> shrinkers map.
> 
> I'm not reading this thread. But is there reason "id" needs to be managed
> using smallest numbers? Can't we use address of shrinker object as "id"
> (which will be sparse bitmap, and would be managed using linked list for now)?

Yes, it's needed to have the smallest numbers, as next patches introduce
per-memcg bitmap containing ids of shrinkers.

Kirill

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

On 18.04.2018 17:14, Tetsuo Handa wrote:
> Kirill Tkhai wrote:
>> The patch introduces shrinker::id number, which is used to enumerate
>> memcg-aware shrinkers. The number start from 0, and the code tries
>> to maintain it as small as possible.
>>
>> This will be used as to represent a memcg-aware shrinkers in memcg
>> shrinkers map.
> 
> I'm not reading this thread. But is there reason "id" needs to be managed
> using smallest numbers? Can't we use address of shrinker object as "id"
> (which will be sparse bitmap, and would be managed using linked list for now)?

Yes, it's needed to have the smallest numbers, as next patches introduce
per-memcg bitmap containing ids of shrinkers.

Kirill

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Tetsuo Handa]

Kirill Tkhai wrote:
> The patch introduces shrinker::id number, which is used to enumerate
> memcg-aware shrinkers. The number start from 0, and the code tries
> to maintain it as small as possible.
> 
> This will be used as to represent a memcg-aware shrinkers in memcg
> shrinkers map.

I'm not reading this thread. But is there reason "id" needs to be managed
using smallest numbers? Can't we use address of shrinker object as "id"
(which will be sparse bitmap, and would be managed using linked list for now)?

Re: [PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Tetsuo Handa]

Kirill Tkhai wrote:
> The patch introduces shrinker::id number, which is used to enumerate
> memcg-aware shrinkers. The number start from 0, and the code tries
> to maintain it as small as possible.
> 
> This will be used as to represent a memcg-aware shrinkers in memcg
> shrinkers map.

I'm not reading this thread. But is there reason "id" needs to be managed
using smallest numbers? Can't we use address of shrinker object as "id"
(which will be sparse bitmap, and would be managed using linked list for now)?

[PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

The patch introduces shrinker::id number, which is used to enumerate
memcg-aware shrinkers. The number start from 0, and the code tries
to maintain it as small as possible.

This will be used as to represent a memcg-aware shrinkers in memcg
shrinkers map.

Signed-off-by: Kirill Tkhai 
---
 include/linux/shrinker.h |2 ++
 mm/vmscan.c  |   51 ++
 2 files changed, 53 insertions(+)

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index a3894918a436..86b651fa2846 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -66,6 +66,8 @@ struct shrinker {
 
/* These are for internal use */
struct list_head list;
+   /* ID in shrinkers_id_idr */
+   int id;
/* objs pending delete, per node */
atomic_long_t *nr_deferred;
 };
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8b920ce3ae02..4f02fe83537e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -169,6 +169,43 @@ unsigned long vm_total_pages;
 static LIST_HEAD(shrinker_list);
 static DECLARE_RWSEM(shrinker_rwsem);
 
+#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
+static DEFINE_IDR(shrinkers_id_idr);
+
+static int add_memcg_shrinker(struct shrinker *shrinker)
+{
+   int id, ret;
+
+   down_write(_rwsem);
+   ret = id = idr_alloc(_id_idr, shrinker, 0, 0, GFP_KERNEL);
+   if (ret < 0)
+   goto unlock;
+   shrinker->id = id;
+   ret = 0;
+unlock:
+   up_write(_rwsem);
+   return ret;
+}
+
+static void del_memcg_shrinker(struct shrinker *shrinker)
+{
+   int id = shrinker->id;
+
+   down_write(_rwsem);
+   idr_remove(_id_idr, id);
+   up_write(_rwsem);
+}
+#else /* CONFIG_MEMCG && !CONFIG_SLOB */
+static int add_memcg_shrinker(struct shrinker *shrinker)
+{
+   return 0;
+}
+
+static void del_memcg_shrinker(struct shrinker *shrinker)
+{
+}
+#endif /* CONFIG_MEMCG && !CONFIG_SLOB */
+
 #ifdef CONFIG_MEMCG
 static bool global_reclaim(struct scan_control *sc)
 {
@@ -306,6 +343,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum 
lru_list lru, int zone
 int register_shrinker(struct shrinker *shrinker)
 {
size_t size = sizeof(*shrinker->nr_deferred);
+   int ret;
 
if (shrinker->flags & SHRINKER_NUMA_AWARE)
size *= nr_node_ids;
@@ -314,10 +352,21 @@ int register_shrinker(struct shrinker *shrinker)
if (!shrinker->nr_deferred)
return -ENOMEM;
 
+   if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
+   ret = add_memcg_shrinker(shrinker);
+   if (ret)
+   goto free_deferred;
+   }
+
down_write(_rwsem);
list_add_tail(>list, _list);
up_write(_rwsem);
return 0;
+
+free_deferred:
+   kfree(shrinker->nr_deferred);
+   shrinker->nr_deferred = NULL;
+   return -ENOMEM;
 }
 EXPORT_SYMBOL(register_shrinker);
 
@@ -328,6 +377,8 @@ void unregister_shrinker(struct shrinker *shrinker)
 {
if (!shrinker->nr_deferred)
return;
+   if (shrinker->flags & SHRINKER_MEMCG_AWARE)
+   del_memcg_shrinker(shrinker);
down_write(_rwsem);
list_del(>list);
up_write(_rwsem);

[PATCH v2 01/12] mm: Assign id to every memcg-aware shrinker

[Kirill Tkhai]

The patch introduces shrinker::id number, which is used to enumerate
memcg-aware shrinkers. The number start from 0, and the code tries
to maintain it as small as possible.

This will be used as to represent a memcg-aware shrinkers in memcg
shrinkers map.

Signed-off-by: Kirill Tkhai 
---
 include/linux/shrinker.h |2 ++
 mm/vmscan.c  |   51 ++
 2 files changed, 53 insertions(+)

diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index a3894918a436..86b651fa2846 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -66,6 +66,8 @@ struct shrinker {
 
/* These are for internal use */
struct list_head list;
+   /* ID in shrinkers_id_idr */
+   int id;
/* objs pending delete, per node */
atomic_long_t *nr_deferred;
 };
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8b920ce3ae02..4f02fe83537e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -169,6 +169,43 @@ unsigned long vm_total_pages;
 static LIST_HEAD(shrinker_list);
 static DECLARE_RWSEM(shrinker_rwsem);
 
+#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
+static DEFINE_IDR(shrinkers_id_idr);
+
+static int add_memcg_shrinker(struct shrinker *shrinker)
+{
+   int id, ret;
+
+   down_write(_rwsem);
+   ret = id = idr_alloc(_id_idr, shrinker, 0, 0, GFP_KERNEL);
+   if (ret < 0)
+   goto unlock;
+   shrinker->id = id;
+   ret = 0;
+unlock:
+   up_write(_rwsem);
+   return ret;
+}
+
+static void del_memcg_shrinker(struct shrinker *shrinker)
+{
+   int id = shrinker->id;
+
+   down_write(_rwsem);
+   idr_remove(_id_idr, id);
+   up_write(_rwsem);
+}
+#else /* CONFIG_MEMCG && !CONFIG_SLOB */
+static int add_memcg_shrinker(struct shrinker *shrinker)
+{
+   return 0;
+}
+
+static void del_memcg_shrinker(struct shrinker *shrinker)
+{
+}
+#endif /* CONFIG_MEMCG && !CONFIG_SLOB */
+
 #ifdef CONFIG_MEMCG
 static bool global_reclaim(struct scan_control *sc)
 {
@@ -306,6 +343,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum 
lru_list lru, int zone
 int register_shrinker(struct shrinker *shrinker)
 {
size_t size = sizeof(*shrinker->nr_deferred);
+   int ret;
 
if (shrinker->flags & SHRINKER_NUMA_AWARE)
size *= nr_node_ids;
@@ -314,10 +352,21 @@ int register_shrinker(struct shrinker *shrinker)
if (!shrinker->nr_deferred)
return -ENOMEM;
 
+   if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
+   ret = add_memcg_shrinker(shrinker);
+   if (ret)
+   goto free_deferred;
+   }
+
down_write(_rwsem);
list_add_tail(>list, _list);
up_write(_rwsem);
return 0;
+
+free_deferred:
+   kfree(shrinker->nr_deferred);
+   shrinker->nr_deferred = NULL;
+   return -ENOMEM;
 }
 EXPORT_SYMBOL(register_shrinker);
 
@@ -328,6 +377,8 @@ void unregister_shrinker(struct shrinker *shrinker)
 {
if (!shrinker->nr_deferred)
return;
+   if (shrinker->flags & SHRINKER_MEMCG_AWARE)
+   del_memcg_shrinker(shrinker);
down_write(_rwsem);
list_del(>list);
up_write(_rwsem);