* [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 3:56 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:56 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li, hughd
Cc: Andrew Morton, linux-mm, linux-kernel
For the implementation (patch 1/3), need fill buffer as full as
possible when buffer space is not enough.
For the caller (patch 2/3, 3/3), need check the return value of
mpol_to_str().
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
fs/proc/task_mmu.c | 16 ++++++++++++++--
mm/mempolicy.c | 14 ++++++++++----
mm/shmem.c | 15 ++++++++++++++-
3 files changed, 38 insertions(+), 7 deletions(-)
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 3:56 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:56 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li
Cc: Andrew Morton, linux-mm, linux-kernel
For the implementation (patch 1/3), need fill buffer as full as
possible when buffer space is not enough.
For the caller (patch 2/3, 3/3), need check the return value of
mpol_to_str().
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
fs/proc/task_mmu.c | 16 ++++++++++++++--
mm/mempolicy.c | 14 ++++++++++----
mm/shmem.c | 15 ++++++++++++++-
3 files changed, 38 insertions(+), 7 deletions(-)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH 1/3] mm/mempolicy.c: still fill buffer as full as possible when buffer space is not enough in mpol_to_str()
2013-08-20 3:56 ` Chen Gang
@ 2013-08-20 3:57 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:57 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li, hughd
Cc: Andrew Morton, linux-mm, linux-kernel
Need still try to fill buffer as full as possible if the buffer space
is not enough, commonly, the caller can bear of it (e.g. print warning
and still continue).
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
mm/mempolicy.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 27022ca..c81b64f 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2800,6 +2800,8 @@ out:
* Convert a mempolicy into a string.
* Returns the number of characters in buffer (if positive)
* or an error (negative)
+ * If the buffer space is not enough, it will return -ENOSPC,
+ * and try to fill the buffer as full as possible.
*/
int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
@@ -2842,11 +2844,10 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
return -EINVAL;
}
+ strlcpy(p, policy_modes[mode], maxlen);
l = strlen(policy_modes[mode]);
if (buffer + maxlen < p + l + 1)
return -ENOSPC;
-
- strcpy(p, policy_modes[mode]);
p += l;
if (flags & MPOL_MODE_FLAGS) {
@@ -2857,10 +2858,15 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
/*
* Currently, the only defined flags are mutually exclusive
*/
- if (flags & MPOL_F_STATIC_NODES)
+ if (flags & MPOL_F_STATIC_NODES) {
p += snprintf(p, buffer + maxlen - p, "static");
- else if (flags & MPOL_F_RELATIVE_NODES)
+ if (buffer + maxlen <= p)
+ return -ENOSPC;
+ } else if (flags & MPOL_F_RELATIVE_NODES) {
p += snprintf(p, buffer + maxlen - p, "relative");
+ if (buffer + maxlen <= p)
+ return -ENOSPC;
+ }
}
if (!nodes_empty(nodes)) {
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 1/3] mm/mempolicy.c: still fill buffer as full as possible when buffer space is not enough in mpol_to_str()
@ 2013-08-20 3:57 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:57 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li
Cc: Andrew Morton, linux-mm, linux-kernel
Need still try to fill buffer as full as possible if the buffer space
is not enough, commonly, the caller can bear of it (e.g. print warning
and still continue).
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
mm/mempolicy.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 27022ca..c81b64f 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2800,6 +2800,8 @@ out:
* Convert a mempolicy into a string.
* Returns the number of characters in buffer (if positive)
* or an error (negative)
+ * If the buffer space is not enough, it will return -ENOSPC,
+ * and try to fill the buffer as full as possible.
*/
int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
@@ -2842,11 +2844,10 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
return -EINVAL;
}
+ strlcpy(p, policy_modes[mode], maxlen);
l = strlen(policy_modes[mode]);
if (buffer + maxlen < p + l + 1)
return -ENOSPC;
-
- strcpy(p, policy_modes[mode]);
p += l;
if (flags & MPOL_MODE_FLAGS) {
@@ -2857,10 +2858,15 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
/*
* Currently, the only defined flags are mutually exclusive
*/
- if (flags & MPOL_F_STATIC_NODES)
+ if (flags & MPOL_F_STATIC_NODES) {
p += snprintf(p, buffer + maxlen - p, "static");
- else if (flags & MPOL_F_RELATIVE_NODES)
+ if (buffer + maxlen <= p)
+ return -ENOSPC;
+ } else if (flags & MPOL_F_RELATIVE_NODES) {
p += snprintf(p, buffer + maxlen - p, "relative");
+ if (buffer + maxlen <= p)
+ return -ENOSPC;
+ }
}
if (!nodes_empty(nodes)) {
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 2/3] fs/proc/task_mmu.c: check the return value of mpol_to_str()
2013-08-20 3:57 ` Chen Gang
@ 2013-08-20 3:58 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:58 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li, hughd
Cc: Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also print related warning when the buffer space is not enough.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
fs/proc/task_mmu.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index a117207..1cb7445 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1359,7 +1359,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
struct mm_struct *mm = vma->vm_mm;
struct mm_walk walk = {};
struct mempolicy *pol;
- int n;
+ int n, ret;
char buffer[50];
if (!mm)
@@ -1376,7 +1376,19 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ ret = mpol_to_str(buffer, sizeof(buffer), pol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ pr_warn("in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ break;
+ default:
+ pr_err("in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), pol);
+ return ret;
+ }
+
mpol_cond_put(pol);
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 2/3] fs/proc/task_mmu.c: check the return value of mpol_to_str()
@ 2013-08-20 3:58 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:58 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li
Cc: Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also print related warning when the buffer space is not enough.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
fs/proc/task_mmu.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index a117207..1cb7445 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1359,7 +1359,7 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
struct mm_struct *mm = vma->vm_mm;
struct mm_walk walk = {};
struct mempolicy *pol;
- int n;
+ int n, ret;
char buffer[50];
if (!mm)
@@ -1376,7 +1376,19 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ ret = mpol_to_str(buffer, sizeof(buffer), pol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ pr_warn("in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ break;
+ default:
+ pr_err("in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), pol);
+ return ret;
+ }
+
mpol_cond_put(pol);
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 3/3] mm/shmem.c: check the return value of mpol_to_str()
2013-08-20 3:58 ` Chen Gang
@ 2013-08-20 3:59 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:59 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li, hughd
Cc: Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also print related warning when the buffer space is not enough.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
mm/shmem.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..eb4eec8 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,24 @@ redirty:
static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
return; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ printk(KERN_WARNING
+ "in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ default:
+ printk(KERN_ERR
+ "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), mpol);
+ return;
+ }
seq_printf(seq, ",mpol=%s", buffer);
}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 3/3] mm/shmem.c: check the return value of mpol_to_str()
@ 2013-08-20 3:59 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 3:59 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, Cyrill Gorcunov,
rientjes, Wanpeng Li
Cc: Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also print related warning when the buffer space is not enough.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
mm/shmem.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..eb4eec8 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,24 @@ redirty:
static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
return; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ printk(KERN_WARNING
+ "in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ default:
+ printk(KERN_ERR
+ "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), mpol);
+ return;
+ }
seq_printf(seq, ",mpol=%s", buffer);
}
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 3:56 ` Chen Gang
@ 2013-08-20 5:30 ` Cyrill Gorcunov
-1 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 5:30 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
> For the implementation (patch 1/3), need fill buffer as full as
> possible when buffer space is not enough.
>
> For the caller (patch 2/3, 3/3), need check the return value of
> mpol_to_str().
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
callers to check that mpol_to_str exit without errors. As far
as I see here are only two users. Something like
show_numa_map
ret = mpol_to_str();
if (ret)
return ret;
shmem_show_mpol
ret = mpol_to_str();
if (ret)
return ret;
sure you'll have to change shmem_show_mpol statement to return int code.
Won't this be more short and convenient?
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 5:30 ` Cyrill Gorcunov
0 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 5:30 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
> For the implementation (patch 1/3), need fill buffer as full as
> possible when buffer space is not enough.
>
> For the caller (patch 2/3, 3/3), need check the return value of
> mpol_to_str().
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
callers to check that mpol_to_str exit without errors. As far
as I see here are only two users. Something like
show_numa_map
ret = mpol_to_str();
if (ret)
return ret;
shmem_show_mpol
ret = mpol_to_str();
if (ret)
return ret;
sure you'll have to change shmem_show_mpol statement to return int code.
Won't this be more short and convenient?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 5:30 ` Cyrill Gorcunov
@ 2013-08-20 5:41 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 5:41 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 01:30 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
>> For the implementation (patch 1/3), need fill buffer as full as
>> possible when buffer space is not enough.
>>
>> For the caller (patch 2/3, 3/3), need check the return value of
>> mpol_to_str().
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>
> Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
> callers to check that mpol_to_str exit without errors. As far
> as I see here are only two users. Something like
>
> show_numa_map
> ret = mpol_to_str();
> if (ret)
> return ret;
>
> shmem_show_mpol
> ret = mpol_to_str();
> if (ret)
> return ret;
>
need "if (ret < 0)" instead of. ;-)
> sure you'll have to change shmem_show_mpol statement to return int code.
> Won't this be more short and convenient?
>
>
Hmm... if return -ENOSPC, in common processing, it still need continue
(but need let outside know about the string truncation).
So I still suggest to give more check for it.
Thanks.
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 5:41 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 5:41 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 01:30 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 11:56:15AM +0800, Chen Gang wrote:
>> For the implementation (patch 1/3), need fill buffer as full as
>> possible when buffer space is not enough.
>>
>> For the caller (patch 2/3, 3/3), need check the return value of
>> mpol_to_str().
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>
> Won't simple check for mpol_to_str() < 0 be enough? IOW fix all
> callers to check that mpol_to_str exit without errors. As far
> as I see here are only two users. Something like
>
> show_numa_map
> ret = mpol_to_str();
> if (ret)
> return ret;
>
> shmem_show_mpol
> ret = mpol_to_str();
> if (ret)
> return ret;
>
need "if (ret < 0)" instead of. ;-)
> sure you'll have to change shmem_show_mpol statement to return int code.
> Won't this be more short and convenient?
>
>
Hmm... if return -ENOSPC, in common processing, it still need continue
(but need let outside know about the string truncation).
So I still suggest to give more check for it.
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 5:41 ` Chen Gang
@ 2013-08-20 6:47 ` Cyrill Gorcunov
-1 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 6:47 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>
> need "if (ret < 0)" instead of. ;-)
sure, it's details
>
> > sure you'll have to change shmem_show_mpol statement to return int code.
> > Won't this be more short and convenient?
> >
> >
>
> Hmm... if return -ENOSPC, in common processing, it still need continue
> (but need let outside know about the string truncation).
>
> So I still suggest to give more check for it.
I still don't like adding additional code like
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ printk(KERN_WARNING
+ "in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ default:
+ printk(KERN_ERR
+ "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), mpol);
+ return;
+ }
this code is pretty neat for debugging purpose I think but in most case (if
only I've not missed something obvious) it simply won't be the case.
Won't somthing like below do the same but with smaller code change?
Note I've not even compiled it but it shows the idea.
---
fs/proc/task_mmu.c | 4 +++-
mm/shmem.c | 17 +++++++++--------
2 files changed, 12 insertions(+), 9 deletions(-)
Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================
--- linux-2.6.git.orig/fs/proc/task_mmu.c
+++ linux-2.6.git/fs/proc/task_mmu.c
@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ n = mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
+ if (n < 0)
+ return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
Index: linux-2.6.git/mm/shmem.c
===================================================================
--- linux-2.6.git.orig/mm/shmem.c
+++ linux-2.6.git/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
#endif /* CONFIG_TMPFS */
static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 6:47 ` Cyrill Gorcunov
0 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 6:47 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>
> need "if (ret < 0)" instead of. ;-)
sure, it's details
>
> > sure you'll have to change shmem_show_mpol statement to return int code.
> > Won't this be more short and convenient?
> >
> >
>
> Hmm... if return -ENOSPC, in common processing, it still need continue
> (but need let outside know about the string truncation).
>
> So I still suggest to give more check for it.
I still don't like adding additional code like
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ switch (ret) {
+ case -ENOSPC:
+ printk(KERN_WARNING
+ "in %s: string is truncated in mpol_to_str().\n",
+ __func__);
+ default:
+ printk(KERN_ERR
+ "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
+ __func__, ret, buffer, sizeof(buffer), mpol);
+ return;
+ }
this code is pretty neat for debugging purpose I think but in most case (if
only I've not missed something obvious) it simply won't be the case.
Won't somthing like below do the same but with smaller code change?
Note I've not even compiled it but it shows the idea.
---
fs/proc/task_mmu.c | 4 +++-
mm/shmem.c | 17 +++++++++--------
2 files changed, 12 insertions(+), 9 deletions(-)
Index: linux-2.6.git/fs/proc/task_mmu.c
===================================================================
--- linux-2.6.git.orig/fs/proc/task_mmu.c
+++ linux-2.6.git/fs/proc/task_mmu.c
@@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ n = mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
+ if (n < 0)
+ return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
Index: linux-2.6.git/mm/shmem.c
===================================================================
--- linux-2.6.git.orig/mm/shmem.c
+++ linux-2.6.git/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
-{
-}
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
#endif /* CONFIG_TMPFS */
static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
@@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 6:47 ` Cyrill Gorcunov
@ 2013-08-20 7:48 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 7:48 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>
>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>> Won't this be more short and convenient?
>>>
>>>
>>
>> Hmm... if return -ENOSPC, in common processing, it still need continue
>> (but need let outside know about the string truncation).
>>
>> So I still suggest to give more check for it.
>
> I still don't like adding additional code like
>
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + switch (ret) {
> + case -ENOSPC:
> + printk(KERN_WARNING
> + "in %s: string is truncated in mpol_to_str().\n",
> + __func__);
> + default:
> + printk(KERN_ERR
> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
> + __func__, ret, buffer, sizeof(buffer), mpol);
> + return;
> + }
>
> this code is pretty neat for debugging purpose I think but in most case (if
> only I've not missed something obvious) it simply won't be the case.
>
For mpol_to_str(), it is for printing string, I suggest to fill buffer
as full as possible like another printing string functions, -ENOSPC is
not critical error, callers may can bear it, and still want to continue.
For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
really not a critical error, they can continue.
For the 'default' error processing:
I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
Thanks.
> Won't somthing like below do the same but with smaller code change?
> Note I've not even compiled it but it shows the idea.
> ---
> fs/proc/task_mmu.c | 4 +++-
> mm/shmem.c | 17 +++++++++--------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> Index: linux-2.6.git/fs/proc/task_mmu.c
> ===================================================================
> --- linux-2.6.git.orig/fs/proc/task_mmu.c
> +++ linux-2.6.git/fs/proc/task_mmu.c
> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
> walk.mm = mm;
>
> pol = get_vma_policy(task, vma, vma->vm_start);
> - mpol_to_str(buffer, sizeof(buffer), pol);
> + n = mpol_to_str(buffer, sizeof(buffer), pol);
> mpol_cond_put(pol);
> + if (n < 0)
> + return n;
>
> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>
> Index: linux-2.6.git/mm/shmem.c
> ===================================================================
> --- linux-2.6.git.orig/mm/shmem.c
> +++ linux-2.6.git/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
> + int ret;
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + return ret;
>
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
> }
> #else /* !CONFIG_NUMA */
> #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> -{
> -}
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
> #endif /* CONFIG_TMPFS */
>
> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
> seq_printf(seq, ",gid=%u",
> from_kgid_munged(&init_user_ns, sbinfo->gid));
> - shmem_show_mpol(seq, sbinfo->mpol);
> - return 0;
> + return shmem_show_mpol(seq, sbinfo->mpol);
> }
> #endif /* CONFIG_TMPFS */
>
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 7:48 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 7:48 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>
>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>> Won't this be more short and convenient?
>>>
>>>
>>
>> Hmm... if return -ENOSPC, in common processing, it still need continue
>> (but need let outside know about the string truncation).
>>
>> So I still suggest to give more check for it.
>
> I still don't like adding additional code like
>
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + switch (ret) {
> + case -ENOSPC:
> + printk(KERN_WARNING
> + "in %s: string is truncated in mpol_to_str().\n",
> + __func__);
> + default:
> + printk(KERN_ERR
> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
> + __func__, ret, buffer, sizeof(buffer), mpol);
> + return;
> + }
>
> this code is pretty neat for debugging purpose I think but in most case (if
> only I've not missed something obvious) it simply won't be the case.
>
For mpol_to_str(), it is for printing string, I suggest to fill buffer
as full as possible like another printing string functions, -ENOSPC is
not critical error, callers may can bear it, and still want to continue.
For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
really not a critical error, they can continue.
For the 'default' error processing:
I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
Thanks.
> Won't somthing like below do the same but with smaller code change?
> Note I've not even compiled it but it shows the idea.
> ---
> fs/proc/task_mmu.c | 4 +++-
> mm/shmem.c | 17 +++++++++--------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> Index: linux-2.6.git/fs/proc/task_mmu.c
> ===================================================================
> --- linux-2.6.git.orig/fs/proc/task_mmu.c
> +++ linux-2.6.git/fs/proc/task_mmu.c
> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
> walk.mm = mm;
>
> pol = get_vma_policy(task, vma, vma->vm_start);
> - mpol_to_str(buffer, sizeof(buffer), pol);
> + n = mpol_to_str(buffer, sizeof(buffer), pol);
> mpol_cond_put(pol);
> + if (n < 0)
> + return n;
>
> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>
> Index: linux-2.6.git/mm/shmem.c
> ===================================================================
> --- linux-2.6.git.orig/mm/shmem.c
> +++ linux-2.6.git/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
> + int ret;
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + return ret;
>
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
> }
> #else /* !CONFIG_NUMA */
> #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> -{
> -}
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
> #endif /* CONFIG_TMPFS */
>
> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
> seq_printf(seq, ",gid=%u",
> from_kgid_munged(&init_user_ns, sbinfo->gid));
> - shmem_show_mpol(seq, sbinfo->mpol);
> - return 0;
> + return shmem_show_mpol(seq, sbinfo->mpol);
> }
> #endif /* CONFIG_TMPFS */
>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 7:48 ` Chen Gang
@ 2013-08-20 7:51 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 7:51 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 03:48 PM, Chen Gang wrote:
> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>
>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>> Won't this be more short and convenient?
>>>>
>>>>
>>>
>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>> (but need let outside know about the string truncation).
>>>
>>> So I still suggest to give more check for it.
>>
>> I still don't like adding additional code like
>>
>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>> + if (ret < 0)
>> + switch (ret) {
>> + case -ENOSPC:
>> + printk(KERN_WARNING
>> + "in %s: string is truncated in mpol_to_str().\n",
>> + __func__);
Oh, that need 'break' in my original patch. :-)
>> + default:
>> + printk(KERN_ERR
>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>> + __func__, ret, buffer, sizeof(buffer), mpol);
>> + return;
>> + }
>>
>> this code is pretty neat for debugging purpose I think but in most case (if
>> only I've not missed something obvious) it simply won't be the case.
>>
>
> For mpol_to_str(), it is for printing string, I suggest to fill buffer
> as full as possible like another printing string functions, -ENOSPC is
> not critical error, callers may can bear it, and still want to continue.
>
> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
> really not a critical error, they can continue.
>
> For the 'default' error processing:
>
> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>
>
> Thanks.
>
>> Won't somthing like below do the same but with smaller code change?
>> Note I've not even compiled it but it shows the idea.
>> ---
>> fs/proc/task_mmu.c | 4 +++-
>> mm/shmem.c | 17 +++++++++--------
>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> Index: linux-2.6.git/fs/proc/task_mmu.c
>> ===================================================================
>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>> +++ linux-2.6.git/fs/proc/task_mmu.c
>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>> walk.mm = mm;
>>
>> pol = get_vma_policy(task, vma, vma->vm_start);
>> - mpol_to_str(buffer, sizeof(buffer), pol);
>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>> mpol_cond_put(pol);
>> + if (n < 0)
>> + return n;
>>
>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>
>> Index: linux-2.6.git/mm/shmem.c
>> ===================================================================
>> --- linux-2.6.git.orig/mm/shmem.c
>> +++ linux-2.6.git/mm/shmem.c
>> @@ -883,16 +883,20 @@ redirty:
>>
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> char buffer[64];
>> + int ret;
>>
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> - return; /* show nothing */
>> + return 0; /* show nothing */
>>
>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>> + if (ret < 0)
>> + return ret;
>>
>> seq_printf(seq, ",mpol=%s", buffer);
>> + return 0;
>> }
>>
>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>> }
>> #else /* !CONFIG_NUMA */
>> #ifdef CONFIG_TMPFS
>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> -{
>> -}
>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>> #endif /* CONFIG_TMPFS */
>>
>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>> seq_printf(seq, ",gid=%u",
>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>> - shmem_show_mpol(seq, sbinfo->mpol);
>> - return 0;
>> + return shmem_show_mpol(seq, sbinfo->mpol);
>> }
>> #endif /* CONFIG_TMPFS */
>>
>>
>>
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 7:51 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 7:51 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 03:48 PM, Chen Gang wrote:
> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>
>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>> Won't this be more short and convenient?
>>>>
>>>>
>>>
>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>> (but need let outside know about the string truncation).
>>>
>>> So I still suggest to give more check for it.
>>
>> I still don't like adding additional code like
>>
>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>> + if (ret < 0)
>> + switch (ret) {
>> + case -ENOSPC:
>> + printk(KERN_WARNING
>> + "in %s: string is truncated in mpol_to_str().\n",
>> + __func__);
Oh, that need 'break' in my original patch. :-)
>> + default:
>> + printk(KERN_ERR
>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>> + __func__, ret, buffer, sizeof(buffer), mpol);
>> + return;
>> + }
>>
>> this code is pretty neat for debugging purpose I think but in most case (if
>> only I've not missed something obvious) it simply won't be the case.
>>
>
> For mpol_to_str(), it is for printing string, I suggest to fill buffer
> as full as possible like another printing string functions, -ENOSPC is
> not critical error, callers may can bear it, and still want to continue.
>
> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
> really not a critical error, they can continue.
>
> For the 'default' error processing:
>
> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>
>
> Thanks.
>
>> Won't somthing like below do the same but with smaller code change?
>> Note I've not even compiled it but it shows the idea.
>> ---
>> fs/proc/task_mmu.c | 4 +++-
>> mm/shmem.c | 17 +++++++++--------
>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> Index: linux-2.6.git/fs/proc/task_mmu.c
>> ===================================================================
>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>> +++ linux-2.6.git/fs/proc/task_mmu.c
>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>> walk.mm = mm;
>>
>> pol = get_vma_policy(task, vma, vma->vm_start);
>> - mpol_to_str(buffer, sizeof(buffer), pol);
>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>> mpol_cond_put(pol);
>> + if (n < 0)
>> + return n;
>>
>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>
>> Index: linux-2.6.git/mm/shmem.c
>> ===================================================================
>> --- linux-2.6.git.orig/mm/shmem.c
>> +++ linux-2.6.git/mm/shmem.c
>> @@ -883,16 +883,20 @@ redirty:
>>
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> char buffer[64];
>> + int ret;
>>
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> - return; /* show nothing */
>> + return 0; /* show nothing */
>>
>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>> + if (ret < 0)
>> + return ret;
>>
>> seq_printf(seq, ",mpol=%s", buffer);
>> + return 0;
>> }
>>
>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>> }
>> #else /* !CONFIG_NUMA */
>> #ifdef CONFIG_TMPFS
>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> -{
>> -}
>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>> #endif /* CONFIG_TMPFS */
>>
>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>> seq_printf(seq, ",gid=%u",
>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>> - shmem_show_mpol(seq, sbinfo->mpol);
>> - return 0;
>> + return shmem_show_mpol(seq, sbinfo->mpol);
>> }
>> #endif /* CONFIG_TMPFS */
>>
>>
>>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 7:51 ` Chen Gang
@ 2013-08-20 8:09 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:09 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 03:51 PM, Chen Gang wrote:
> On 08/20/2013 03:48 PM, Chen Gang wrote:
>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>
>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>> Won't this be more short and convenient?
>>>>>
>>>>>
>>>>
>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>> (but need let outside know about the string truncation).
>>>>
>>>> So I still suggest to give more check for it.
>>>
>>> I still don't like adding additional code like
>>>
>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + if (ret < 0)
>>> + switch (ret) {
>>> + case -ENOSPC:
>>> + printk(KERN_WARNING
>>> + "in %s: string is truncated in mpol_to_str().\n",
>>> + __func__);
>
> Oh, that need 'break' in my original patch. :-)
>
>>> + default:
>>> + printk(KERN_ERR
>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>> + return;
>>> + }
>>>
>>> this code is pretty neat for debugging purpose I think but in most case (if
>>> only I've not missed something obvious) it simply won't be the case.
>>>
>>
>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>> as full as possible like another printing string functions, -ENOSPC is
>> not critical error, callers may can bear it, and still want to continue.
>>
>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>> really not a critical error, they can continue.
>>
>> For the 'default' error processing:
>>
>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>
>>
>> Thanks.
>>
Oh, for '-ENOSPC', it means critical error, it is my fault.
So, for simplify thinking and implementation, use your patch below is OK
to me (but I suggest to print error information in the none return value
function).
:-)
>>> Won't somthing like below do the same but with smaller code change?
>>> Note I've not even compiled it but it shows the idea.
>>> ---
>>> fs/proc/task_mmu.c | 4 +++-
>>> mm/shmem.c | 17 +++++++++--------
>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>> walk.mm = mm;
>>>
>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>> mpol_cond_put(pol);
>>> + if (n < 0)
>>> + return n;
>>>
>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>
>>> Index: linux-2.6.git/mm/shmem.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/mm/shmem.c
>>> +++ linux-2.6.git/mm/shmem.c
>>> @@ -883,16 +883,20 @@ redirty:
>>>
>>> #ifdef CONFIG_NUMA
>>> #ifdef CONFIG_TMPFS
>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> {
>>> char buffer[64];
>>> + int ret;
>>>
>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>> - return; /* show nothing */
>>> + return 0; /* show nothing */
>>>
>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> seq_printf(seq, ",mpol=%s", buffer);
>>> + return 0;
>>> }
>>>
>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>> }
>>> #else /* !CONFIG_NUMA */
>>> #ifdef CONFIG_TMPFS
>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> -{
>>> -}
>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>> #endif /* CONFIG_TMPFS */
>>>
>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>> seq_printf(seq, ",gid=%u",
>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>> - return 0;
>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>> }
>>> #endif /* CONFIG_TMPFS */
>>>
>>>
>>>
>>
>>
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 8:09 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:09 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 03:51 PM, Chen Gang wrote:
> On 08/20/2013 03:48 PM, Chen Gang wrote:
>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>
>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>> Won't this be more short and convenient?
>>>>>
>>>>>
>>>>
>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>> (but need let outside know about the string truncation).
>>>>
>>>> So I still suggest to give more check for it.
>>>
>>> I still don't like adding additional code like
>>>
>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + if (ret < 0)
>>> + switch (ret) {
>>> + case -ENOSPC:
>>> + printk(KERN_WARNING
>>> + "in %s: string is truncated in mpol_to_str().\n",
>>> + __func__);
>
> Oh, that need 'break' in my original patch. :-)
>
>>> + default:
>>> + printk(KERN_ERR
>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>> + return;
>>> + }
>>>
>>> this code is pretty neat for debugging purpose I think but in most case (if
>>> only I've not missed something obvious) it simply won't be the case.
>>>
>>
>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>> as full as possible like another printing string functions, -ENOSPC is
>> not critical error, callers may can bear it, and still want to continue.
>>
>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>> really not a critical error, they can continue.
>>
>> For the 'default' error processing:
>>
>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>
>>
>> Thanks.
>>
Oh, for '-ENOSPC', it means critical error, it is my fault.
So, for simplify thinking and implementation, use your patch below is OK
to me (but I suggest to print error information in the none return value
function).
:-)
>>> Won't somthing like below do the same but with smaller code change?
>>> Note I've not even compiled it but it shows the idea.
>>> ---
>>> fs/proc/task_mmu.c | 4 +++-
>>> mm/shmem.c | 17 +++++++++--------
>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>> walk.mm = mm;
>>>
>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>> mpol_cond_put(pol);
>>> + if (n < 0)
>>> + return n;
>>>
>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>
>>> Index: linux-2.6.git/mm/shmem.c
>>> ===================================================================
>>> --- linux-2.6.git.orig/mm/shmem.c
>>> +++ linux-2.6.git/mm/shmem.c
>>> @@ -883,16 +883,20 @@ redirty:
>>>
>>> #ifdef CONFIG_NUMA
>>> #ifdef CONFIG_TMPFS
>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> {
>>> char buffer[64];
>>> + int ret;
>>>
>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>> - return; /* show nothing */
>>> + return 0; /* show nothing */
>>>
>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + if (ret < 0)
>>> + return ret;
>>>
>>> seq_printf(seq, ",mpol=%s", buffer);
>>> + return 0;
>>> }
>>>
>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>> }
>>> #else /* !CONFIG_NUMA */
>>> #ifdef CONFIG_TMPFS
>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>> -{
>>> -}
>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>> #endif /* CONFIG_TMPFS */
>>>
>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>> seq_printf(seq, ",gid=%u",
>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>> - return 0;
>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>> }
>>> #endif /* CONFIG_TMPFS */
>>>
>>>
>>>
>>
>>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 8:09 ` Chen Gang
@ 2013-08-20 8:13 ` Chen Gang F T
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang F T @ 2013-08-20 8:13 UTC (permalink / raw)
To: Chen Gang
Cc: Cyrill Gorcunov, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 04:09 PM, Chen Gang wrote:
> On 08/20/2013 03:51 PM, Chen Gang wrote:
>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>
>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>> Won't this be more short and convenient?
>>>>>>
>>>>>>
>>>>>
>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>> (but need let outside know about the string truncation).
>>>>>
>>>>> So I still suggest to give more check for it.
>>>>
>>>> I still don't like adding additional code like
>>>>
>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + if (ret < 0)
>>>> + switch (ret) {
>>>> + case -ENOSPC:
>>>> + printk(KERN_WARNING
>>>> + "in %s: string is truncated in mpol_to_str().\n",
>>>> + __func__);
>>
>> Oh, that need 'break' in my original patch. :-)
>>
>>>> + default:
>>>> + printk(KERN_ERR
>>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>>> + return;
>>>> + }
>>>>
>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>> only I've not missed something obvious) it simply won't be the case.
>>>>
>>>
>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>> as full as possible like another printing string functions, -ENOSPC is
>>> not critical error, callers may can bear it, and still want to continue.
>>>
>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>> really not a critical error, they can continue.
>>>
>>> For the 'default' error processing:
>>>
>>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>
>>>
>>> Thanks.
>>>
>
> Oh, for '-ENOSPC', it means critical error, it is my fault.
>
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).
>
> :-)
>
>>>> Won't somthing like below do the same but with smaller code change?
>>>> Note I've not even compiled it but it shows the idea.
>>>> ---
>>>> fs/proc/task_mmu.c | 4 +++-
>>>> mm/shmem.c | 17 +++++++++--------
>>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>> walk.mm = mm;
>>>>
>>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>> mpol_cond_put(pol);
>>>> + if (n < 0)
>>>> + return n;
>>>>
>>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>
>>>> Index: linux-2.6.git/mm/shmem.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>> +++ linux-2.6.git/mm/shmem.c
>>>> @@ -883,16 +883,20 @@ redirty:
>>>>
>>>> #ifdef CONFIG_NUMA
>>>> #ifdef CONFIG_TMPFS
>>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> {
>>>> char buffer[64];
>>>> + int ret;
>>>>
>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>> - return; /* show nothing */
>>>> + return 0; /* show nothing */
>>>>
>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + if (ret < 0)
>>>> + return ret;
>>>>
>>>> seq_printf(seq, ",mpol=%s", buffer);
>>>> + return 0;
>>>> }
>>>>
>>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>>> }
>>>> #else /* !CONFIG_NUMA */
>>>> #ifdef CONFIG_TMPFS
>>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> -{
>>>> -}
>>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>>> #endif /* CONFIG_TMPFS */
>>>>
>>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>>> seq_printf(seq, ",gid=%u",
>>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>>> - return 0;
>>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>>> }
>>>> #endif /* CONFIG_TMPFS */
>>>>
>>>>
>>>>
Oh, you have done, sorry.
>>>
>>>
>>
>>
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 8:13 ` Chen Gang F T
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang F T @ 2013-08-20 8:13 UTC (permalink / raw)
To: Chen Gang
Cc: Cyrill Gorcunov, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 04:09 PM, Chen Gang wrote:
> On 08/20/2013 03:51 PM, Chen Gang wrote:
>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>
>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>> Won't this be more short and convenient?
>>>>>>
>>>>>>
>>>>>
>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>> (but need let outside know about the string truncation).
>>>>>
>>>>> So I still suggest to give more check for it.
>>>>
>>>> I still don't like adding additional code like
>>>>
>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + if (ret < 0)
>>>> + switch (ret) {
>>>> + case -ENOSPC:
>>>> + printk(KERN_WARNING
>>>> + "in %s: string is truncated in mpol_to_str().\n",
>>>> + __func__);
>>
>> Oh, that need 'break' in my original patch. :-)
>>
>>>> + default:
>>>> + printk(KERN_ERR
>>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>>> + return;
>>>> + }
>>>>
>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>> only I've not missed something obvious) it simply won't be the case.
>>>>
>>>
>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>> as full as possible like another printing string functions, -ENOSPC is
>>> not critical error, callers may can bear it, and still want to continue.
>>>
>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>> really not a critical error, they can continue.
>>>
>>> For the 'default' error processing:
>>>
>>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>
>>>
>>> Thanks.
>>>
>
> Oh, for '-ENOSPC', it means critical error, it is my fault.
>
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).
>
> :-)
>
>>>> Won't somthing like below do the same but with smaller code change?
>>>> Note I've not even compiled it but it shows the idea.
>>>> ---
>>>> fs/proc/task_mmu.c | 4 +++-
>>>> mm/shmem.c | 17 +++++++++--------
>>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>>
>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>> walk.mm = mm;
>>>>
>>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>> mpol_cond_put(pol);
>>>> + if (n < 0)
>>>> + return n;
>>>>
>>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>
>>>> Index: linux-2.6.git/mm/shmem.c
>>>> ===================================================================
>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>> +++ linux-2.6.git/mm/shmem.c
>>>> @@ -883,16 +883,20 @@ redirty:
>>>>
>>>> #ifdef CONFIG_NUMA
>>>> #ifdef CONFIG_TMPFS
>>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> {
>>>> char buffer[64];
>>>> + int ret;
>>>>
>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>> - return; /* show nothing */
>>>> + return 0; /* show nothing */
>>>>
>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + if (ret < 0)
>>>> + return ret;
>>>>
>>>> seq_printf(seq, ",mpol=%s", buffer);
>>>> + return 0;
>>>> }
>>>>
>>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>>> }
>>>> #else /* !CONFIG_NUMA */
>>>> #ifdef CONFIG_TMPFS
>>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>> -{
>>>> -}
>>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>>> #endif /* CONFIG_TMPFS */
>>>>
>>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>>> seq_printf(seq, ",gid=%u",
>>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>>> - return 0;
>>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>>> }
>>>> #endif /* CONFIG_TMPFS */
>>>>
>>>>
>>>>
Oh, you have done, sorry.
>>>
>>>
>>
>>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 8:13 ` Chen Gang F T
@ 2013-08-20 8:20 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:20 UTC (permalink / raw)
To: Chen Gang F T
Cc: Cyrill Gorcunov, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
Sorry for reply multiple times to bother many members.
It seems, I am tired, and need have a rest today. :-(
On 08/20/2013 04:13 PM, Chen Gang F T wrote:
> On 08/20/2013 04:09 PM, Chen Gang wrote:
>> On 08/20/2013 03:51 PM, Chen Gang wrote:
>>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>>
>>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>>> Won't this be more short and convenient?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>>> (but need let outside know about the string truncation).
>>>>>>
>>>>>> So I still suggest to give more check for it.
>>>>>
>>>>> I still don't like adding additional code like
>>>>>
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + switch (ret) {
>>>>> + case -ENOSPC:
>>>>> + printk(KERN_WARNING
>>>>> + "in %s: string is truncated in mpol_to_str().\n",
>>>>> + __func__);
>>>
>>> Oh, that need 'break' in my original patch. :-)
>>>
>>>>> + default:
>>>>> + printk(KERN_ERR
>>>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>>>> + return;
>>>>> + }
>>>>>
>>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>>> only I've not missed something obvious) it simply won't be the case.
>>>>>
>>>>
>>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>>> as full as possible like another printing string functions, -ENOSPC is
>>>> not critical error, callers may can bear it, and still want to continue.
>>>>
>>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>>> really not a critical error, they can continue.
>>>>
>>>> For the 'default' error processing:
>>>>
>>>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>>
>>>>
>>>> Thanks.
>>>>
>>
>> Oh, for '-ENOSPC', it means critical error, it is my fault.
>>
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>>
>> :-)
>>
>>>>> Won't somthing like below do the same but with smaller code change?
>>>>> Note I've not even compiled it but it shows the idea.
>>>>> ---
>>>>> fs/proc/task_mmu.c | 4 +++-
>>>>> mm/shmem.c | 17 +++++++++--------
>>>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>>>
>>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>>> walk.mm = mm;
>>>>>
>>>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> mpol_cond_put(pol);
>>>>> + if (n < 0)
>>>>> + return n;
>>>>>
>>>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>>
>>>>> Index: linux-2.6.git/mm/shmem.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>>> +++ linux-2.6.git/mm/shmem.c
>>>>> @@ -883,16 +883,20 @@ redirty:
>>>>>
>>>>> #ifdef CONFIG_NUMA
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> {
>>>>> char buffer[64];
>>>>> + int ret;
>>>>>
>>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>>> - return; /* show nothing */
>>>>> + return 0; /* show nothing */
>>>>>
>>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>>
>>>>> seq_printf(seq, ",mpol=%s", buffer);
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>>>> }
>>>>> #else /* !CONFIG_NUMA */
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> -{
>>>>> -}
>>>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>>>> seq_printf(seq, ",gid=%u",
>>>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>>>> - return 0;
>>>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>>>> }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>>
>>>>>
>
> Oh, you have done, sorry.
>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 8:20 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:20 UTC (permalink / raw)
To: Chen Gang F T
Cc: Cyrill Gorcunov, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
Sorry for reply multiple times to bother many members.
It seems, I am tired, and need have a rest today. :-(
On 08/20/2013 04:13 PM, Chen Gang F T wrote:
> On 08/20/2013 04:09 PM, Chen Gang wrote:
>> On 08/20/2013 03:51 PM, Chen Gang wrote:
>>> On 08/20/2013 03:48 PM, Chen Gang wrote:
>>>> On 08/20/2013 02:47 PM, Cyrill Gorcunov wrote:
>>>>> On Tue, Aug 20, 2013 at 01:41:40PM +0800, Chen Gang wrote:
>>>>>>
>>>>>>> sure you'll have to change shmem_show_mpol statement to return int code.
>>>>>>> Won't this be more short and convenient?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Hmm... if return -ENOSPC, in common processing, it still need continue
>>>>>> (but need let outside know about the string truncation).
>>>>>>
>>>>>> So I still suggest to give more check for it.
>>>>>
>>>>> I still don't like adding additional code like
>>>>>
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + switch (ret) {
>>>>> + case -ENOSPC:
>>>>> + printk(KERN_WARNING
>>>>> + "in %s: string is truncated in mpol_to_str().\n",
>>>>> + __func__);
>>>
>>> Oh, that need 'break' in my original patch. :-)
>>>
>>>>> + default:
>>>>> + printk(KERN_ERR
>>>>> + "in %s: call mpol_to_str() fail, errcode: %d. buffer: %p, size: %zu, pol: %p\n",
>>>>> + __func__, ret, buffer, sizeof(buffer), mpol);
>>>>> + return;
>>>>> + }
>>>>>
>>>>> this code is pretty neat for debugging purpose I think but in most case (if
>>>>> only I've not missed something obvious) it simply won't be the case.
>>>>>
>>>>
>>>> For mpol_to_str(), it is for printing string, I suggest to fill buffer
>>>> as full as possible like another printing string functions, -ENOSPC is
>>>> not critical error, callers may can bear it, and still want to continue.
>>>>
>>>> For 2 callers, I still suggest to process '-ENOSPC' and continue, it is
>>>> really not a critical error, they can continue.
>>>>
>>>> For the 'default' error processing:
>>>>
>>>> I still suggest to 'printk' in shmem_show_mpol(), because when failure occurs, it has no return value to mark the failure to upper caller.
>>>> Hmm... but for show_numa_map(), may remove the 'printk', only return the error code is OK. :-)
>>>>
>>>>
>>>> Thanks.
>>>>
>>
>> Oh, for '-ENOSPC', it means critical error, it is my fault.
>>
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>>
>> :-)
>>
>>>>> Won't somthing like below do the same but with smaller code change?
>>>>> Note I've not even compiled it but it shows the idea.
>>>>> ---
>>>>> fs/proc/task_mmu.c | 4 +++-
>>>>> mm/shmem.c | 17 +++++++++--------
>>>>> 2 files changed, 12 insertions(+), 9 deletions(-)
>>>>>
>>>>> Index: linux-2.6.git/fs/proc/task_mmu.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/fs/proc/task_mmu.c
>>>>> +++ linux-2.6.git/fs/proc/task_mmu.c
>>>>> @@ -1402,8 +1402,10 @@ static int show_numa_map(struct seq_file
>>>>> walk.mm = mm;
>>>>>
>>>>> pol = get_vma_policy(task, vma, vma->vm_start);
>>>>> - mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> + n = mpol_to_str(buffer, sizeof(buffer), pol);
>>>>> mpol_cond_put(pol);
>>>>> + if (n < 0)
>>>>> + return n;
>>>>>
>>>>> seq_printf(m, "%08lx %s", vma->vm_start, buffer);
>>>>>
>>>>> Index: linux-2.6.git/mm/shmem.c
>>>>> ===================================================================
>>>>> --- linux-2.6.git.orig/mm/shmem.c
>>>>> +++ linux-2.6.git/mm/shmem.c
>>>>> @@ -883,16 +883,20 @@ redirty:
>>>>>
>>>>> #ifdef CONFIG_NUMA
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> {
>>>>> char buffer[64];
>>>>> + int ret;
>>>>>
>>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>>> - return; /* show nothing */
>>>>> + return 0; /* show nothing */
>>>>>
>>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>>
>>>>> seq_printf(seq, ",mpol=%s", buffer);
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>>>>> @@ -951,9 +955,7 @@ static struct page *shmem_alloc_page(gfp
>>>>> }
>>>>> #else /* !CONFIG_NUMA */
>>>>> #ifdef CONFIG_TMPFS
>>>>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>>>> -{
>>>>> -}
>>>>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol) { return 0; }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>> static inline struct page *shmem_swapin(swp_entry_t swap, gfp_t gfp,
>>>>> @@ -2577,8 +2579,7 @@ static int shmem_show_options(struct seq
>>>>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>>>>> seq_printf(seq, ",gid=%u",
>>>>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>>>>> - shmem_show_mpol(seq, sbinfo->mpol);
>>>>> - return 0;
>>>>> + return shmem_show_mpol(seq, sbinfo->mpol);
>>>>> }
>>>>> #endif /* CONFIG_TMPFS */
>>>>>
>>>>>
>>>>>
>
> Oh, you have done, sorry.
>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 8:09 ` Chen Gang
@ 2013-08-20 8:25 ` Cyrill Gorcunov
-1 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 8:25 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).
Chen, I'm not going to dive into this area now, too busy with other stuff
sorry, so if you consider my preliminary draft patch useful -- pick it up,
modify it, test it and send to lkml then (just CC me).
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 8:25 ` Cyrill Gorcunov
0 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-20 8:25 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
> So, for simplify thinking and implementation, use your patch below is OK
> to me (but I suggest to print error information in the none return value
> function).
Chen, I'm not going to dive into this area now, too busy with other stuff
sorry, so if you consider my preliminary draft patch useful -- pick it up,
modify it, test it and send to lkml then (just CC me).
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
2013-08-20 8:25 ` Cyrill Gorcunov
@ 2013-08-20 8:31 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:31 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 04:25 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>
> Chen, I'm not going to dive into this area now, too busy with other stuff
> sorry, so if you consider my preliminary draft patch useful -- pick it up,
> modify it, test it and send to lkml then (just CC me).
>
>
OK, I will do tomorrow. :-)
Thanks.
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str()
@ 2013-08-20 8:31 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-20 8:31 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/20/2013 04:25 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 20, 2013 at 04:09:22PM +0800, Chen Gang wrote:
>> So, for simplify thinking and implementation, use your patch below is OK
>> to me (but I suggest to print error information in the none return value
>> function).
>
> Chen, I'm not going to dive into this area now, too busy with other stuff
> sorry, so if you consider my preliminary draft patch useful -- pick it up,
> modify it, test it and send to lkml then (just CC me).
>
>
OK, I will do tomorrow. :-)
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
2013-08-20 8:25 ` Cyrill Gorcunov
@ 2013-08-21 2:21 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:21 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
fs/proc/task_mmu.c | 4 +++-
mm/shmem.c | 16 ++++++++++------
2 files changed, 13 insertions(+), 7 deletions(-)
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
@ 2013-08-21 2:21 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:21 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
fs/proc/task_mmu.c | 4 +++-
mm/shmem.c | 16 ++++++++++------
2 files changed, 13 insertions(+), 7 deletions(-)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH 1/3] fs/proc/task_mmu.c: check the return value of mpol_to_str()
2013-08-21 2:21 ` Chen Gang
@ 2013-08-21 2:22 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:22 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
The failure return need after mpol_cond_put() to match get_vma_policy().
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
fs/proc/task_mmu.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 107d026..d87f5da 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1376,8 +1376,10 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ n = mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
+ if (n < 0)
+ return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 1/3] fs/proc/task_mmu.c: check the return value of mpol_to_str()
@ 2013-08-21 2:22 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:22 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
The failure return need after mpol_cond_put() to match get_vma_policy().
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
fs/proc/task_mmu.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 107d026..d87f5da 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1376,8 +1376,10 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- mpol_to_str(buffer, sizeof(buffer), pol);
+ n = mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
+ if (n < 0)
+ return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
2013-08-21 2:22 ` Chen Gang
@ 2013-08-21 2:23 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:23 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
Let shmem_show_mpol() return value, since it may fail.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..e59d332 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,17 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
mpol_to_str(buffer, sizeof(buffer), mpol);
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +952,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
+ return 0;
}
#endif /* CONFIG_TMPFS */
@@ -2578,8 +2580,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
@ 2013-08-21 2:23 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:23 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
Let shmem_show_mpol() return value, since it may fail.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 75010ba..e59d332 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,17 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
mpol_to_str(buffer, sizeof(buffer), mpol);
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +952,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
+ return 0;
}
#endif /* CONFIG_TMPFS */
@@ -2578,8 +2580,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 3/3] mm/shmem.c: check the return value of mpol_to_str()
2013-08-21 2:23 ` Chen Gang
@ 2013-08-21 2:24 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:24 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index e59d332..ae5112f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,14 @@ redirty:
static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
return 0;
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH 3/3] mm/shmem.c: check the return value of mpol_to_str()
@ 2013-08-21 2:24 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 2:24 UTC (permalink / raw)
To: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, Andrew Morton, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Cc: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index e59d332..ae5112f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -886,11 +886,14 @@ redirty:
static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
return 0;
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
2013-08-21 2:21 ` Chen Gang
@ 2013-08-21 5:31 ` Cyrill Gorcunov
-1 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-21 5:31 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
> check about it, or buffer may not be zero based, and next seq_printf()
> will cause issue.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Looks good to me, thanks!
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
@ 2013-08-21 5:31 ` Cyrill Gorcunov
0 siblings, 0 replies; 92+ messages in thread
From: Cyrill Gorcunov @ 2013-08-21 5:31 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
> check about it, or buffer may not be zero based, and next seq_printf()
> will cause issue.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
Looks good to me, thanks!
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
2013-08-21 5:31 ` Cyrill Gorcunov
@ 2013-08-21 5:48 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 5:48 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Chen Gang, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/21/2013 01:31 PM, Cyrill Gorcunov wrote:
> On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
>> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
>> check about it, or buffer may not be zero based, and next seq_printf()
>> will cause issue.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
>
> Looks good to me, thanks!
>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
>
Thanks.
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 0/3] mm: shmem: check the return value of mpol_to_str()
@ 2013-08-21 5:48 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-21 5:48 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Chen Gang, Mel Gorman, kosaki.motohiro, riel, hughd, xemul,
rientjes, Wanpeng Li, Andrew Morton, linux-mm, linux-kernel
On 08/21/2013 01:31 PM, Cyrill Gorcunov wrote:
> On Wed, Aug 21, 2013 at 10:21:22AM +0800, Chen Gang wrote:
>> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
>> check about it, or buffer may not be zero based, and next seq_printf()
>> will cause issue.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> Cc: Cyrill Gorcunov <gorcunov@gmail.com>
>
> Looks good to me, thanks!
>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
>
Thanks.
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
2013-08-21 2:23 ` Chen Gang
@ 2013-08-21 22:03 ` Andrew Morton
-1 siblings, 0 replies; 92+ messages in thread
From: Andrew Morton @ 2013-08-21 22:03 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> Let shmem_show_mpol() return value, since it may fail.
>
This patch has no effect.
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,17 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> mpol_to_str(buffer, sizeof(buffer), mpol);
Perhaps you meant to check the mpol_to_str() return value here.
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
@ 2013-08-21 22:03 ` Andrew Morton
0 siblings, 0 replies; 92+ messages in thread
From: Andrew Morton @ 2013-08-21 22:03 UTC (permalink / raw)
To: Chen Gang
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> Let shmem_show_mpol() return value, since it may fail.
>
This patch has no effect.
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,17 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> mpol_to_str(buffer, sizeof(buffer), mpol);
Perhaps you meant to check the mpol_to_str() return value here.
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
2013-08-21 22:03 ` Andrew Morton
@ 2013-08-22 0:52 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-22 0:52 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
On 08/22/2013 06:03 AM, Andrew Morton wrote:
> On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:
>
>> Let shmem_show_mpol() return value, since it may fail.
>>
>
> This patch has no effect.
>
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -883,16 +883,17 @@ redirty:
>>
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> char buffer[64];
>>
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> - return; /* show nothing */
>> + return 0; /* show nothing */
>>
>> mpol_to_str(buffer, sizeof(buffer), mpol);
>
> Perhaps you meant to check the mpol_to_str() return value here.
>
Yes, I will merge them together (merge Patch 2/3 and Patch 3/3).
>> seq_printf(seq, ",mpol=%s", buffer);
>> + return 0;
>> }
>>
>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>
>
>
Thanks.
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value.
@ 2013-08-22 0:52 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-22 0:52 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
On 08/22/2013 06:03 AM, Andrew Morton wrote:
> On Wed, 21 Aug 2013 10:23:58 +0800 Chen Gang <gang.chen@asianux.com> wrote:
>
>> Let shmem_show_mpol() return value, since it may fail.
>>
>
> This patch has no effect.
>
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -883,16 +883,17 @@ redirty:
>>
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> char buffer[64];
>>
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> - return; /* show nothing */
>> + return 0; /* show nothing */
>>
>> mpol_to_str(buffer, sizeof(buffer), mpol);
>
> Perhaps you meant to check the mpol_to_str() return value here.
>
Yes, I will merge them together (merge Patch 2/3 and Patch 3/3).
>> seq_printf(seq, ",mpol=%s", buffer);
>> + return 0;
>> }
>>
>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>
>
>
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH] mm/shmem.c: check the return value of mpol_to_str()
2013-08-22 0:52 ` Chen Gang
@ 2013-08-22 1:04 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-22 1:04 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also need let shmem_show_mpol() return value, since it may fail.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index f00c1c1..b4d44db 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
+ return 0;
}
#endif /* CONFIG_TMPFS */
@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
1.7.7.6
^ permalink raw reply related [flat|nested] 92+ messages in thread
* [PATCH] mm/shmem.c: check the return value of mpol_to_str()
@ 2013-08-22 1:04 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-08-22 1:04 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also need let shmem_show_mpol() return value, since it may fail.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index f00c1c1..b4d44db 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
+ return 0;
}
#endif /* CONFIG_TMPFS */
@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [PATCH] mm/shmem.c: check the return value of mpol_to_str()
2013-08-22 1:04 ` Chen Gang
@ 2013-09-03 5:32 ` Chen Gang
-1 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-03 5:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
Hello Maintainers:
Please help check this patch, when you have time.
If it need additional test, please let me know, I should try (better to
provide some suggestions for test).
Thanks.
On 08/22/2013 09:04 AM, Chen Gang wrote:
> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
> check about it, or buffer may not be zero based, and next seq_printf()
> will cause issue.
>
> Also need let shmem_show_mpol() return value, since it may fail.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> mm/shmem.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f00c1c1..b4d44db 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
> + int ret;
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + return ret;
>
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
> }
> #else /* !CONFIG_NUMA */
> #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> + return 0;
> }
> #endif /* CONFIG_TMPFS */
>
> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
> seq_printf(seq, ",gid=%u",
> from_kgid_munged(&init_user_ns, sbinfo->gid));
> - shmem_show_mpol(seq, sbinfo->mpol);
> - return 0;
> + return shmem_show_mpol(seq, sbinfo->mpol);
> }
> #endif /* CONFIG_TMPFS */
>
>
--
Chen Gang
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH] mm/shmem.c: check the return value of mpol_to_str()
@ 2013-09-03 5:32 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-03 5:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Mel Gorman, kosaki.motohiro, riel, hughd, xemul, rientjes,
Wanpeng Li, Cyrill Gorcunov, linux-mm, linux-kernel
Hello Maintainers:
Please help check this patch, when you have time.
If it need additional test, please let me know, I should try (better to
provide some suggestions for test).
Thanks.
On 08/22/2013 09:04 AM, Chen Gang wrote:
> mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
> check about it, or buffer may not be zero based, and next seq_printf()
> will cause issue.
>
> Also need let shmem_show_mpol() return value, since it may fail.
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> mm/shmem.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f00c1c1..b4d44db 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
> + int ret;
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
> + if (ret < 0)
> + return ret;
>
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
> }
> #else /* !CONFIG_NUMA */
> #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> + return 0;
> }
> #endif /* CONFIG_TMPFS */
>
> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
> seq_printf(seq, ",gid=%u",
> from_kgid_munged(&init_user_ns, sbinfo->gid));
> - shmem_show_mpol(seq, sbinfo->mpol);
> - return 0;
> + return shmem_show_mpol(seq, sbinfo->mpol);
> }
> #endif /* CONFIG_TMPFS */
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-08-22 1:04 ` Chen Gang
(?)
(?)
@ 2013-09-05 0:24 ` Chen Gang
2013-09-09 20:30 ` David Rientjes
2013-09-25 2:58 ` David Rientjes
-1 siblings, 2 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-05 0:24 UTC (permalink / raw)
To: KOSAKI Motohiro, riel, hughd, xemul, rientjes, Wanpeng Li
Cc: Cyrill Gorcunov, linux-mm, Andrew Morton
mpol_to_str() may fail, and not fill the buffer (e.g. -EINVAL), so need
check about it, or buffer may not be zero based, and next seq_printf()
will cause issue.
Also need let shmem_show_mpol() return value, since it may fail.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/shmem.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index f00c1c1..b4d44db 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -883,16 +883,20 @@ redirty:
#ifdef CONFIG_NUMA
#ifdef CONFIG_TMPFS
-static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
char buffer[64];
+ int ret;
if (!mpol || mpol->mode == MPOL_DEFAULT)
- return; /* show nothing */
+ return 0; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ ret = mpol_to_str(buffer, sizeof(buffer), mpol);
+ if (ret < 0)
+ return ret;
seq_printf(seq, ",mpol=%s", buffer);
+ return 0;
}
static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
@@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
}
#else /* !CONFIG_NUMA */
#ifdef CONFIG_TMPFS
-static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
+static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
{
+ return 0;
}
#endif /* CONFIG_TMPFS */
@@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
seq_printf(seq, ",gid=%u",
from_kgid_munged(&init_user_ns, sbinfo->gid));
- shmem_show_mpol(seq, sbinfo->mpol);
- return 0;
+ return shmem_show_mpol(seq, sbinfo->mpol);
}
#endif /* CONFIG_TMPFS */
--
1.7.7.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-05 0:24 ` [PATCH v2] " Chen Gang
@ 2013-09-09 20:30 ` David Rientjes
2013-09-10 0:47 ` Chen Gang
2013-09-25 2:58 ` David Rientjes
1 sibling, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-09 20:30 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On Thu, 5 Sep 2013, Chen Gang wrote:
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f00c1c1..b4d44db 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -883,16 +883,20 @@ redirty:
>
> #ifdef CONFIG_NUMA
> #ifdef CONFIG_TMPFS
> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> char buffer[64];
> + int ret;
>
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> - return; /* show nothing */
> + return 0; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
I was wondering how mpol_to_str() could fail given a pointer to a stack
allocated buffer, so I checked and it happens if the mempolicy mode isn't
known or the buffer isn't long enough.
I think it would be better to keep mpol_to_str() returning void, and hence
avoiding the need for this patch, and make it so it cannot fail. If the
mode is invalid, just store a 0 to the buffer (or "unknown"); and if
maxlen isn't large enough, make it a compile-time error (let's avoid
trying to be fancy and allocating less than 64 bytes on the stack if a
given context is known to have short mempolicy strings).
> + if (ret < 0)
> + return ret;
>
> seq_printf(seq, ",mpol=%s", buffer);
> + return 0;
> }
>
> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
> }
> #else /* !CONFIG_NUMA */
> #ifdef CONFIG_TMPFS
> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> {
> + return 0;
> }
> #endif /* CONFIG_TMPFS */
>
> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
> seq_printf(seq, ",gid=%u",
> from_kgid_munged(&init_user_ns, sbinfo->gid));
> - shmem_show_mpol(seq, sbinfo->mpol);
> - return 0;
> + return shmem_show_mpol(seq, sbinfo->mpol);
> }
> #endif /* CONFIG_TMPFS */
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-09 20:30 ` David Rientjes
@ 2013-09-10 0:47 ` Chen Gang
2013-09-10 6:43 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-10 0:47 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On 09/10/2013 04:30 AM, David Rientjes wrote:
> On Thu, 5 Sep 2013, Chen Gang wrote:
>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index f00c1c1..b4d44db 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -883,16 +883,20 @@ redirty:
>>
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> -static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> char buffer[64];
>> + int ret;
>>
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> - return; /* show nothing */
>> + return 0; /* show nothing */
>>
>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>> + ret = mpol_to_str(buffer, sizeof(buffer), mpol);
>
> I was wondering how mpol_to_str() could fail given a pointer to a stack
> allocated buffer, so I checked and it happens if the mempolicy mode isn't
> known or the buffer isn't long enough.
>
Yeah.
> I think it would be better to keep mpol_to_str() returning void, and hence
> avoiding the need for this patch, and make it so it cannot fail. If the
> mode is invalid, just store a 0 to the buffer (or "unknown"); and if
> maxlen isn't large enough, make it a compile-time error (let's avoid
> trying to be fancy and allocating less than 64 bytes on the stack if a
> given context is known to have short mempolicy strings).
>
Hmm... at least, like most of print functions, it need return a value
to tell the length it writes, so in my opinion, I still suggest it can
return a value.
For common printing functions, caller knows about the string format and
all parameters, and also can control them, so for callee, it is not
'quite polite' to return any failures to caller. :-)
But for our function, caller may not know about the string format and
parameters' details, so callee has duty to check and process them:
e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".
Thanks.
>> + if (ret < 0)
>> + return ret;
>>
>> seq_printf(seq, ",mpol=%s", buffer);
>> + return 0;
>> }
>>
>> static struct mempolicy *shmem_get_sbmpol(struct shmem_sb_info *sbinfo)
>> @@ -951,8 +955,9 @@ static struct page *shmem_alloc_page(gfp_t gfp,
>> }
>> #else /* !CONFIG_NUMA */
>> #ifdef CONFIG_TMPFS
>> -static inline void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> +static inline int shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> {
>> + return 0;
>> }
>> #endif /* CONFIG_TMPFS */
>>
>> @@ -2555,8 +2560,7 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
>> if (!gid_eq(sbinfo->gid, GLOBAL_ROOT_GID))
>> seq_printf(seq, ",gid=%u",
>> from_kgid_munged(&init_user_ns, sbinfo->gid));
>> - shmem_show_mpol(seq, sbinfo->mpol);
>> - return 0;
>> + return shmem_show_mpol(seq, sbinfo->mpol);
>> }
>> #endif /* CONFIG_TMPFS */
>>
>
>
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-10 0:47 ` Chen Gang
@ 2013-09-10 6:43 ` David Rientjes
2013-09-10 7:01 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-10 6:43 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On Tue, 10 Sep 2013, Chen Gang wrote:
> > I think it would be better to keep mpol_to_str() returning void, and hence
> > avoiding the need for this patch, and make it so it cannot fail. If the
> > mode is invalid, just store a 0 to the buffer (or "unknown"); and if
> > maxlen isn't large enough, make it a compile-time error (let's avoid
> > trying to be fancy and allocating less than 64 bytes on the stack if a
> > given context is known to have short mempolicy strings).
> >
>
> Hmm... at least, like most of print functions, it need return a value
> to tell the length it writes, so in my opinion, I still suggest it can
> return a value.
>
Why? It can just store the string into the buffer pointed to by the
char *buffer and terminate it appropriately while taking care that it
doesn't exceed maxlen. Why does the caller need to know the number of
bytes written? If it really does, you could just do strlen(buffer).
If there's a real reason for it, then that's fine, I just think it can be
made to always succeed and never return < 0. (And why is nobody checking
the return value today if it's so necessary?)
> For common printing functions, caller knows about the string format and
> all parameters, and also can control them, so for callee, it is not
> 'quite polite' to return any failures to caller. :-)
>
> But for our function, caller may not know about the string format and
> parameters' details, so callee has duty to check and process them:
>
> e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".
>
Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
If the struct mempolicy really has a bad mode, then just store "unknown"
or store a 0. If maxlen is insufficient for the longest possible string
stored by mpol_to_str(), then it should be a compile-time error.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-10 6:43 ` David Rientjes
@ 2013-09-10 7:01 ` Chen Gang
2013-09-12 0:33 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-10 7:01 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On 09/10/2013 02:43 PM, David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
>
>>> I think it would be better to keep mpol_to_str() returning void, and hence
>>> avoiding the need for this patch, and make it so it cannot fail. If the
>>> mode is invalid, just store a 0 to the buffer (or "unknown"); and if
>>> maxlen isn't large enough, make it a compile-time error (let's avoid
>>> trying to be fancy and allocating less than 64 bytes on the stack if a
>>> given context is known to have short mempolicy strings).
>>>
>>
>> Hmm... at least, like most of print functions, it need return a value
>> to tell the length it writes, so in my opinion, I still suggest it can
>> return a value.
>>
>
> Why? It can just store the string into the buffer pointed to by the
> char *buffer and terminate it appropriately while taking care that it
> doesn't exceed maxlen. Why does the caller need to know the number of
> bytes written? If it really does, you could just do strlen(buffer).
>
> If there's a real reason for it, then that's fine, I just think it can be
> made to always succeed and never return < 0. (And why is nobody checking
> the return value today if it's so necessary?)
>
For common printing functions: sprintf(), snprintf(), scnprintf().
For some of specific printing functions: drivers/usb/host/uhci-debug.c.
at least they can let caller easy use.
>> For common printing functions, caller knows about the string format and
>> all parameters, and also can control them, so for callee, it is not
>> 'quite polite' to return any failures to caller. :-)
>>
>> But for our function, caller may not know about the string format and
>> parameters' details, so callee has duty to check and process them:
>>
>> e.g. "if related parameter is invalid, it is neccessary to notifiy to caller".
>>
>
> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
> If the struct mempolicy really has a bad mode, then just store "unknown"
> or store a 0. If maxlen is insufficient for the longest possible string
> stored by mpol_to_str(), then it should be a compile-time error.
>
>
Hmm... what you said sounds reasonable if mpol_to_str() is a normal
static funciton (only used within a file).
For extern function, callee (inside) can not assume anything of caller
(outside) beyond the interface. So if failure occurs, better to report
to caller only, and let caller to check what to do next.
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-10 7:01 ` Chen Gang
@ 2013-09-12 0:33 ` David Rientjes
2013-09-12 2:19 ` KOSAKI Motohiro
2013-09-12 3:02 ` Chen Gang
0 siblings, 2 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-12 0:33 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On Tue, 10 Sep 2013, Chen Gang wrote:
> > Why? It can just store the string into the buffer pointed to by the
> > char *buffer and terminate it appropriately while taking care that it
> > doesn't exceed maxlen. Why does the caller need to know the number of
> > bytes written? If it really does, you could just do strlen(buffer).
> >
> > If there's a real reason for it, then that's fine, I just think it can be
> > made to always succeed and never return < 0. (And why is nobody checking
> > the return value today if it's so necessary?)
> >
>
> For common printing functions: sprintf(), snprintf(), scnprintf().
>
> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>
> at least they can let caller easy use.
>
Nobody needs mpol_to_str() to return the number of characters written,
period. It's one of the most trivial functions you're going to see in the
mempolicy code, it takes a pointer to a buffer and it stores characters to
it for display. Nobody is going to use it for anything else. Let's not
overcomplicate this trivial function.
> > Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
> > If the struct mempolicy really has a bad mode, then just store "unknown"
> > or store a 0. If maxlen is insufficient for the longest possible string
> > stored by mpol_to_str(), then it should be a compile-time error.
> >
> >
>
> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
> static funciton (only used within a file).
>
> For extern function, callee (inside) can not assume anything of caller
> (outside) beyond the interface. So if failure occurs, better to report
> to caller only, and let caller to check what to do next.
>
Are you just preaching about the best practices of software engineering?
mpol_to_str() should never fail at runtime, plain and simple. If somebody
introduces a new mode and doesn't update it to print correctly, let's not
fail the read(). Let's just print "unknown". And if someone passes too
small of a buffer, break it at compile time so it gets noticed and fixed.
I guarantee you that any kernel developer who writes code to call
mpol_to_str() will be happy it never fails at runtime. Really.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 0:33 ` David Rientjes
@ 2013-09-12 2:19 ` KOSAKI Motohiro
2013-09-12 3:13 ` Chen Gang
2013-09-13 21:12 ` David Rientjes
2013-09-12 3:02 ` Chen Gang
1 sibling, 2 replies; 92+ messages in thread
From: KOSAKI Motohiro @ 2013-09-12 2:19 UTC (permalink / raw)
To: David Rientjes
Cc: Chen Gang, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton, kosaki.motohiro
(9/11/13 8:33 PM), David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
>
>>> Why? It can just store the string into the buffer pointed to by the
>>> char *buffer and terminate it appropriately while taking care that it
>>> doesn't exceed maxlen. Why does the caller need to know the number of
>>> bytes written? If it really does, you could just do strlen(buffer).
>>>
>>> If there's a real reason for it, then that's fine, I just think it can be
>>> made to always succeed and never return < 0. (And why is nobody checking
>>> the return value today if it's so necessary?)
>>>
>>
>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>
>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>
>> at least they can let caller easy use.
>>
>
> Nobody needs mpol_to_str() to return the number of characters written,
> period. It's one of the most trivial functions you're going to see in the
> mempolicy code, it takes a pointer to a buffer and it stores characters to
> it for display. Nobody is going to use it for anything else. Let's not
> overcomplicate this trivial function.
>
>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
>>> If the struct mempolicy really has a bad mode, then just store "unknown"
>>> or store a 0. If maxlen is insufficient for the longest possible string
>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>
>>>
>>
>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>> static funciton (only used within a file).
>>
>> For extern function, callee (inside) can not assume anything of caller
>> (outside) beyond the interface. So if failure occurs, better to report
>> to caller only, and let caller to check what to do next.
>>
>
> Are you just preaching about the best practices of software engineering?
> mpol_to_str() should never fail at runtime, plain and simple. If somebody
> introduces a new mode and doesn't update it to print correctly, let's not
> fail the read(). Let's just print "unknown". And if someone passes too
> small of a buffer, break it at compile time so it gets noticed and fixed.
>
> I guarantee you that any kernel developer who writes code to call
> mpol_to_str() will be happy it never fails at runtime. Really.
Agreed. Even though we don't change mpol_to_str() interface, please just
add BUG_ON into shmem_show_mpol(). It is much simpler than current proposal.
At least, currently mpol_to_str() already have following assertion. I mean,
the code assume every developer know maximum length of mempolicy. I have no
seen any reason to bring addional complication to shmem area.
/*
* Sanity check: room for longest mode, flag and some nodes
*/
VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
Thanks.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 0:33 ` David Rientjes
2013-09-12 2:19 ` KOSAKI Motohiro
@ 2013-09-12 3:02 ` Chen Gang
2013-09-12 18:19 ` KOSAKI Motohiro
2013-09-13 21:14 ` David Rientjes
1 sibling, 2 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-12 3:02 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On 09/12/2013 08:33 AM, David Rientjes wrote:
> On Tue, 10 Sep 2013, Chen Gang wrote:
>
>>> Why? It can just store the string into the buffer pointed to by the
>>> char *buffer and terminate it appropriately while taking care that it
>>> doesn't exceed maxlen. Why does the caller need to know the number of
>>> bytes written? If it really does, you could just do strlen(buffer).
>>>
>>> If there's a real reason for it, then that's fine, I just think it can be
>>> made to always succeed and never return < 0. (And why is nobody checking
>>> the return value today if it's so necessary?)
>>>
>>
>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>
>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>
>> at least they can let caller easy use.
>>
>
> Nobody needs mpol_to_str() to return the number of characters written,
> period. It's one of the most trivial functions you're going to see in the
> mempolicy code, it takes a pointer to a buffer and it stores characters to
> it for display. Nobody is going to use it for anything else. Let's not
> overcomplicate this trivial function.
>
Most of tools functions in kernel are return a value to let the caller
easy use, e.g. memset() which much simpler than our case, still return a
value.
>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is valid :)
>>> If the struct mempolicy really has a bad mode, then just store "unknown"
>>> or store a 0. If maxlen is insufficient for the longest possible string
>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>
>>>
>>
>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>> static funciton (only used within a file).
>>
>> For extern function, callee (inside) can not assume anything of caller
>> (outside) beyond the interface. So if failure occurs, better to report
>> to caller only, and let caller to check what to do next.
>>
>
> Are you just preaching about the best practices of software engineering?
> mpol_to_str() should never fail at runtime, plain and simple. If somebody
> introduces a new mode and doesn't update it to print correctly, let's not
> fail the read(). Let's just print "unknown". And if someone passes too
> small of a buffer, break it at compile time so it gets noticed and fixed.
>
For normal static function, that sounds reasonable.
> I guarantee you that any kernel developer who writes code to call
> mpol_to_str() will be happy it never fails at runtime. Really.
>
>
Hmm... for extern function, at present, maybe you can guarantee, but may
not always in the future. And "Code is mainly for making readers 'happy'
(e.g version mergers), not writers".
For extern function which more than 50 lines (used by 2 sub-systems), it
is strange for readers to find it no return value, also strange to let
*BUG_ON() on the extern function's input parameters directly.
If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
< 0)", that will be more clearer to all members (need this patch do like
it? :-) ).
BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
replaced by returning -EINVAL.
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 2:19 ` KOSAKI Motohiro
@ 2013-09-12 3:13 ` Chen Gang
2013-09-13 21:12 ` David Rientjes
1 sibling, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-12 3:13 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: David Rientjes, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/12/2013 10:19 AM, KOSAKI Motohiro wrote:
> (9/11/13 8:33 PM), David Rientjes wrote:
>> On Tue, 10 Sep 2013, Chen Gang wrote:
>>
>>>> Why? It can just store the string into the buffer pointed to by the
>>>> char *buffer and terminate it appropriately while taking care that it
>>>> doesn't exceed maxlen. Why does the caller need to know the number of
>>>> bytes written? If it really does, you could just do strlen(buffer).
>>>>
>>>> If there's a real reason for it, then that's fine, I just think it
>>>> can be
>>>> made to always succeed and never return < 0. (And why is nobody
>>>> checking
>>>> the return value today if it's so necessary?)
>>>>
>>>
>>> For common printing functions: sprintf(), snprintf(), scnprintf().
>>>
>>> For some of specific printing functions: drivers/usb/host/uhci-debug.c.
>>>
>>> at least they can let caller easy use.
>>>
>>
>> Nobody needs mpol_to_str() to return the number of characters written,
>> period. It's one of the most trivial functions you're going to see in
>> the
>> mempolicy code, it takes a pointer to a buffer and it stores
>> characters to
>> it for display. Nobody is going to use it for anything else. Let's not
>> overcomplicate this trivial function.
>>
>>>> Nobody is using mpol_to_str() to determine if a mempolicy mode is
>>>> valid :)
>>>> If the struct mempolicy really has a bad mode, then just store
>>>> "unknown"
>>>> or store a 0. If maxlen is insufficient for the longest possible
>>>> string
>>>> stored by mpol_to_str(), then it should be a compile-time error.
>>>>
>>>>
>>>
>>> Hmm... what you said sounds reasonable if mpol_to_str() is a normal
>>> static funciton (only used within a file).
>>>
>>> For extern function, callee (inside) can not assume anything of caller
>>> (outside) beyond the interface. So if failure occurs, better to report
>>> to caller only, and let caller to check what to do next.
>>>
>>
>> Are you just preaching about the best practices of software engineering?
>> mpol_to_str() should never fail at runtime, plain and simple. If
>> somebody
>> introduces a new mode and doesn't update it to print correctly, let's not
>> fail the read(). Let's just print "unknown". And if someone passes too
>> small of a buffer, break it at compile time so it gets noticed and fixed.
>>
>> I guarantee you that any kernel developer who writes code to call
>> mpol_to_str() will be happy it never fails at runtime. Really.
>
> Agreed. Even though we don't change mpol_to_str() interface, please just
> add BUG_ON into shmem_show_mpol(). It is much simpler than current
> proposal.
>
Hmm... that is simpler and clearer for writers, but may not for readers.
> At least, currently mpol_to_str() already have following assertion. I mean,
> the code assume every developer know maximum length of mempolicy. I have no
> seen any reason to bring addional complication to shmem area.
>
>
> /*
> * Sanity check: room for longest mode, flag and some nodes
> */
> VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>
> Thanks.
>
Hmm... *BUG_ON() is for protecting the OS continue blindly, can we be
sure: "in current condition, OS is continuing blindly?"
If an extern function's parameter is invalid, it mainly means caller
incorrectly use the function (which need return -EINVAL), not means "the
OS is continuing blindly".
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 3:02 ` Chen Gang
@ 2013-09-12 18:19 ` KOSAKI Motohiro
2013-09-13 2:23 ` Chen Gang
2013-09-13 21:14 ` David Rientjes
1 sibling, 1 reply; 92+ messages in thread
From: KOSAKI Motohiro @ 2013-09-12 18:19 UTC (permalink / raw)
To: Chen Gang
Cc: David Rientjes, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton, kosaki.motohiro
> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
> replaced by returning -EINVAL.
Nope. mpol_to_str() is not carefully designed since it was born. It
doesn't have a way to get proper buffer size. That said, the function
assume all caller know proper buffer size. So, just adding EINVAL
doesn't solve anything. we need to add a way to get proper buffer length
at least if we take your way. However it is overengineering because
current all caller doesn't need it.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 18:19 ` KOSAKI Motohiro
@ 2013-09-13 2:23 ` Chen Gang
2013-09-13 16:50 ` KOSAKI Motohiro
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-13 2:23 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: David Rientjes, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/13/2013 02:19 AM, KOSAKI Motohiro wrote:
>> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
>> replaced by returning -EINVAL.
>
> Nope. mpol_to_str() is not carefully designed since it was born. It
> doesn't have a way to get proper buffer size. That said, the function
> assume all caller know proper buffer size. So, just adding EINVAL
> doesn't solve anything. we need to add a way to get proper buffer length
> at least if we take your way. However it is overengineering because
> current all caller doesn't need it.
>
That sounds reasonable.
Hmm... but I still believe there must be a fixing way to satisfy us all.
Please check the patch below whether can satisfy us all, thanks.
-------------------------------patch begin-----------------------------
mm/shmem.c: use VM_BUG_ON() for mpol_to_str() when it fails.
mpol_to_str() is an extern function which may return a failure. But in
our case, it should not,
If it really return a failure, that means current kernel is continuing
blindly (e.g. some kernel structures are corrupted), should be stopped
as soon as possible.
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
mm/shmem.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 8612a95..3f81120 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
if (!mpol || mpol->mode == MPOL_DEFAULT)
return; /* show nothing */
- mpol_to_str(buffer, sizeof(buffer), mpol);
+ VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
seq_printf(seq, ",mpol=%s", buffer);
}
--
1.7.7.6
-------------------------------patch end-------------------------------
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-13 2:23 ` Chen Gang
@ 2013-09-13 16:50 ` KOSAKI Motohiro
2013-09-16 2:55 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: KOSAKI Motohiro @ 2013-09-13 16:50 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, David Rientjes, KOSAKI Motohiro, riel, hughd,
xemul, Wanpeng Li, Cyrill Gorcunov, linux-mm, Andrew Morton
> ---
> mm/shmem.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 8612a95..3f81120 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> if (!mpol || mpol->mode == MPOL_DEFAULT)
> return; /* show nothing */
>
> - mpol_to_str(buffer, sizeof(buffer), mpol);
> + VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
NAK. VM_BUG_ON is a kind of assertion. It erase the contents if CONFIG_DEBUG_VM not set.
An argument of assertion should not have any side effect.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 2:19 ` KOSAKI Motohiro
2013-09-12 3:13 ` Chen Gang
@ 2013-09-13 21:12 ` David Rientjes
2013-09-14 2:51 ` KOSAKI Motohiro
1 sibling, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-13 21:12 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: Chen Gang, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:
> At least, currently mpol_to_str() already have following assertion. I mean,
> the code assume every developer know maximum length of mempolicy. I have no
> seen any reason to bring addional complication to shmem area.
>
>
> /*
> * Sanity check: room for longest mode, flag and some nodes
> */
> VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>
No need to make it a runtime error, the value passed as maxlen is a
constant, as is the use of sizeof(buffer), so the value is known at
compile-time. You can make this a BUILD_BUG_ON() if you are creative.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-12 3:02 ` Chen Gang
2013-09-12 18:19 ` KOSAKI Motohiro
@ 2013-09-13 21:14 ` David Rientjes
2013-09-16 3:17 ` Chen Gang
1 sibling, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-13 21:14 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On Thu, 12 Sep 2013, Chen Gang wrote:
> Hmm... for extern function, at present, maybe you can guarantee, but may
> not always in the future. And "Code is mainly for making readers 'happy'
> (e.g version mergers), not writers".
>
> For extern function which more than 50 lines (used by 2 sub-systems), it
> is strange for readers to find it no return value, also strange to let
> *BUG_ON() on the extern function's input parameters directly.
>
> If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
> < 0)", that will be more clearer to all members (need this patch do like
> it? :-) ).
>
>
> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
> replaced by returning -EINVAL.
>
Are you reading my emails?
I'm asking for a compile-time error if the maxlen passed to mpol_to_str()
is too small; it's a constant value and can be evaluated at compile-time.
Then mpol_to_str() can return void if you simply store "unknown" when it's
an unknown mode.
Sheesh.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-13 21:12 ` David Rientjes
@ 2013-09-14 2:51 ` KOSAKI Motohiro
2013-09-16 3:27 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: KOSAKI Motohiro @ 2013-09-14 2:51 UTC (permalink / raw)
To: rientjes
Cc: kosaki.motohiro, gang.chen, kosaki.motohiro, riel, hughd, xemul,
liwanp, gorcunov, linux-mm, akpm
On 9/13/2013 5:12 PM, David Rientjes wrote:
> On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:
>
>> At least, currently mpol_to_str() already have following assertion. I mean,
>> the code assume every developer know maximum length of mempolicy. I have no
>> seen any reason to bring addional complication to shmem area.
>>
>>
>> /*
>> * Sanity check: room for longest mode, flag and some nodes
>> */
>> VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>>
>
> No need to make it a runtime error, the value passed as maxlen is a
> constant, as is the use of sizeof(buffer), so the value is known at
> compile-time. You can make this a BUILD_BUG_ON() if you are creative.
Making compile time error brings us another complication. I'd like to
keep just one line assertion.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-13 16:50 ` KOSAKI Motohiro
@ 2013-09-16 2:55 ` Chen Gang
2013-09-16 16:16 ` KOSAKI Motohiro
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-16 2:55 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: David Rientjes, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>> ---
>> mm/shmem.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 8612a95..3f81120 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>> struct mempolicy *mpol)
>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>> return; /* show nothing */
>>
>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>> + VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>
> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
> CONFIG_DEBUG_VM not set.
> An argument of assertion should not have any side effect.
>
>
>
Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
instead of "VM_BUG_ON(mpol_to_str() < 0);".
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-13 21:14 ` David Rientjes
@ 2013-09-16 3:17 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-16 3:17 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li, Cyrill Gorcunov,
linux-mm, Andrew Morton
On 09/14/2013 05:14 AM, David Rientjes wrote:
> On Thu, 12 Sep 2013, Chen Gang wrote:
>
>> Hmm... for extern function, at present, maybe you can guarantee, but may
>> not always in the future. And "Code is mainly for making readers 'happy'
>> (e.g version mergers), not writers".
>>
>> For extern function which more than 50 lines (used by 2 sub-systems), it
>> is strange for readers to find it no return value, also strange to let
>> *BUG_ON() on the extern function's input parameters directly.
>>
>> If one caller wants to treat failure as BUG, can "*BUG_ON(mpol_to_str()
>> < 0)", that will be more clearer to all members (need this patch do like
>> it? :-) ).
>>
>>
>> BTW: in my opinion, within mpol_to_str(), the VM_BUG_ON() need be
>> replaced by returning -EINVAL.
>>
>
> Are you reading my emails?
>
Yes.
> I'm asking for a compile-time error if the maxlen passed to mpol_to_str()
> is too small; it's a constant value and can be evaluated at compile-time.
> Then mpol_to_str() can return void if you simply store "unknown" when it's
> an unknown mode.
>
Are/were you saying: 'gcc' can realize an extern functions' input
parameter whether is a constant??
If so, excuse me, I really did not quite understand what you were
saying, but I am still trying to understand.
As far as I know:
mpol_to_str() is called by 2 areas, which will input different maxlen.
for a none-inline function, compiler treats parameters as variables.
for ANSI C compiler, for function's parameter, "array == pointer".
Hmm... maybe you see 'sizeof()'? if so, we also need notice: "multiple
callers only call one callee with there different 'sizeof()'", callee
has to treat these 'sizeof()' values as variable, not constant.
If I am still misunderstanding, please say more with details, thanks.
> Sheesh.
>
>
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-14 2:51 ` KOSAKI Motohiro
@ 2013-09-16 3:27 ` Chen Gang
2013-09-16 20:13 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-16 3:27 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: rientjes, kosaki.motohiro, riel, hughd, xemul, liwanp, gorcunov,
linux-mm, akpm
On 09/14/2013 10:51 AM, KOSAKI Motohiro wrote:
> On 9/13/2013 5:12 PM, David Rientjes wrote:
>> On Wed, 11 Sep 2013, KOSAKI Motohiro wrote:
>>
>>> At least, currently mpol_to_str() already have following assertion. I mean,
>>> the code assume every developer know maximum length of mempolicy. I have no
>>> seen any reason to bring addional complication to shmem area.
>>>
>>>
>>> /*
>>> * Sanity check: room for longest mode, flag and some nodes
>>> */
>>> VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
>>>
>>
>> No need to make it a runtime error, the value passed as maxlen is a
>> constant, as is the use of sizeof(buffer), so the value is known at
>> compile-time. You can make this a BUILD_BUG_ON() if you are creative.
>
> Making compile time error brings us another complication. I'd like to
> keep just one line assertion.
>
Hmm... I am not quite sure: a C compiler is clever enough to know about
that.
At least, for ANSI C definition, the C compiler has no duty to know
about that.
And it is not for an optimization, either, so I guess the C compiler has
no enought interests to support this features (know about that).
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-16 2:55 ` Chen Gang
@ 2013-09-16 16:16 ` KOSAKI Motohiro
2013-09-17 1:10 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: KOSAKI Motohiro @ 2013-09-16 16:16 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, David Rientjes, KOSAKI Motohiro, riel, hughd,
xemul, Wanpeng Li, Cyrill Gorcunov, linux-mm, Andrew Morton
(9/15/13 10:55 PM), Chen Gang wrote:
> On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>>> ---
>>> mm/shmem.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 8612a95..3f81120 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>>> struct mempolicy *mpol)
>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>> return; /* show nothing */
>>>
>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>> + VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>>
>> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
>> CONFIG_DEBUG_VM not set.
>> An argument of assertion should not have any side effect.
>
> Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
> instead of "VM_BUG_ON(mpol_to_str() < 0);".
BUG_ON() is safe. but I still don't like it. As far as I heard, Google
changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
Please treat an assertion as assertion. Not any other something.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-16 3:27 ` Chen Gang
@ 2013-09-16 20:13 ` David Rientjes
2013-09-17 0:45 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-16 20:13 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, kosaki.motohiro, riel, hughd, xemul, liwanp,
gorcunov, linux-mm, akpm
On Mon, 16 Sep 2013, Chen Gang wrote:
> Hmm... I am not quite sure: a C compiler is clever enough to know about
> that.
>
> At least, for ANSI C definition, the C compiler has no duty to know
> about that.
>
> And it is not for an optimization, either, so I guess the C compiler has
> no enought interests to support this features (know about that).
>
What on earth are we talking about in this thread?
Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in
mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
64) and then calls __mpol_to_str().
Modify __mpol_to_str() to store "unknown" when mpol->mode does not match
any known MPOL_* constant.
Both functions can now return void.
This is like a ten line diff. Seriously.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-16 20:13 ` David Rientjes
@ 2013-09-17 0:45 ` Chen Gang
2013-09-17 22:51 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-17 0:45 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, kosaki.motohiro, riel, hughd, xemul, liwanp,
gorcunov, linux-mm, akpm
On 09/17/2013 04:13 AM, David Rientjes wrote:
> On Mon, 16 Sep 2013, Chen Gang wrote:
>
>> Hmm... I am not quite sure: a C compiler is clever enough to know about
>> that.
>>
>> At least, for ANSI C definition, the C compiler has no duty to know
>> about that.
>>
>> And it is not for an optimization, either, so I guess the C compiler has
>> no enought interests to support this features (know about that).
>>
>
> What on earth are we talking about in this thread?
>
??
> Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in
> mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
> 64) and then calls __mpol_to_str().
>
> Modify __mpol_to_str() to store "unknown" when mpol->mode does not match
> any known MPOL_* constant.
>
Can we be sure 'maxlen' should not be less than 64? For show_numa_map()
in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?
> Both functions can now return void.
>
> This is like a ten line diff. Seriously.
>
>
Can we be sure that our output contents are always less than 64 bytes?
Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?
Hmm... If assume what you said above was always correct: "we are always
sure 64 bytes is enough, and 'maxlen' should be never less than 64".
It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
(and also still need check 64 memory bondary and '\0' within mpol_to_str).
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-16 16:16 ` KOSAKI Motohiro
@ 2013-09-17 1:10 ` Chen Gang
2013-09-17 22:53 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-17 1:10 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: David Rientjes, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/17/2013 12:16 AM, KOSAKI Motohiro wrote:
> (9/15/13 10:55 PM), Chen Gang wrote:
>> On 09/14/2013 12:50 AM, KOSAKI Motohiro wrote:
>>>> ---
>>>> mm/shmem.c | 2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>>> index 8612a95..3f81120 100644
>>>> --- a/mm/shmem.c
>>>> +++ b/mm/shmem.c
>>>> @@ -890,7 +890,7 @@ static void shmem_show_mpol(struct seq_file *seq,
>>>> struct mempolicy *mpol)
>>>> if (!mpol || mpol->mode == MPOL_DEFAULT)
>>>> return; /* show nothing */
>>>>
>>>> - mpol_to_str(buffer, sizeof(buffer), mpol);
>>>> + VM_BUG_ON(mpol_to_str(buffer, sizeof(buffer), mpol) < 0);
>>>
>>> NAK. VM_BUG_ON is a kind of assertion. It erase the contents if
>>> CONFIG_DEBUG_VM not set.
>>> An argument of assertion should not have any side effect.
>>
>> Oh, really it is. In my opinion, need use "BUG_ON(mpol_to_str() < 0)"
>> instead of "VM_BUG_ON(mpol_to_str() < 0);".
>
> BUG_ON() is safe. but I still don't like it. As far as I heard, Google
> changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
> Please treat an assertion as assertion. Not any other something.
>
Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
"mm/" is a common sub-system (not architecture specific), so when we
use BUG_ON(), we already 'express' our 'opinion' enough to readers.
And some architectures/users really can customize/config 'BUG/BUG_ON'
(they can implement it by themselves, or 'nop').
If they choose 'nop', they can let code size smaller (also may faster),
but they (not we) also have duty to face related risk: "when we find OS
is continuing blindly, we do not let it stop".
Related information for BUG in "init/Kconfig" (which BUG_ON based on):
config BUG
bool "BUG() support" if EXPERT
default y
help
Disabling this option eliminates support for BUG and WARN, reducing
the size of your kernel image and potentially quietly ignoring
numerous fatal conditions. You should only consider disabling this
option for embedded systems with no facilities for reporting errors.
Just say Y.
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-17 0:45 ` Chen Gang
@ 2013-09-17 22:51 ` David Rientjes
2013-09-18 1:20 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-17 22:51 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, kosaki.motohiro, riel, hughd, xemul, liwanp,
gorcunov, linux-mm, akpm
On Tue, 17 Sep 2013, Chen Gang wrote:
> > Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in
> > mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
> > 64) and then calls __mpol_to_str().
> >
> > Modify __mpol_to_str() to store "unknown" when mpol->mode does not match
> > any known MPOL_* constant.
> >
>
> Can we be sure 'maxlen' should not be less than 64? For show_numa_map()
> in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?
>
Whatever the max string length is that can be stored by mpol_to_str()
preferably rounded to the nearest power of two.
> Can we be sure that our output contents are always less than 64 bytes?
> Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?
>
You can determine the maximum string length by looking at the
implementation of mpol_to_str().
> Hmm... If assume what you said above was always correct: "we are always
> sure 64 bytes is enough, and 'maxlen' should be never less than 64".
>
> It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
> (and also still need check 64 memory bondary and '\0' within mpol_to_str).
>
That's ridiculous, kernel developers who call mpol_to_str() aren't idiots.
I think at this point it will just be best if I propose a patch and ask
for it to be merged into the -mm tree rather than continue this thread.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-17 1:10 ` Chen Gang
@ 2013-09-17 22:53 ` David Rientjes
2013-09-18 1:37 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-17 22:53 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On Tue, 17 Sep 2013, Chen Gang wrote:
> > BUG_ON() is safe. but I still don't like it. As far as I heard, Google
> > changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
> > Please treat an assertion as assertion. Not any other something.
> >
Google does not disable BUG_ON(), sheesh.
> Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
> "mm/" is a common sub-system (not architecture specific), so when we
> use BUG_ON(), we already 'express' our 'opinion' enough to readers.
>
That's ridiculous, we're not going to panic the kernel at runtime because
a buffer is too small. Make it a compile-time error like I suggested so
we catch this before we even build the kernel.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-17 22:51 ` David Rientjes
@ 2013-09-18 1:20 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-18 1:20 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, kosaki.motohiro, riel, hughd, xemul, liwanp,
gorcunov, linux-mm, akpm
On 09/18/2013 06:51 AM, David Rientjes wrote:
> On Tue, 17 Sep 2013, Chen Gang wrote:
>
>>> Rename mpol_to_str() to __mpol_to_str(). Make a static inline function in
>>> mempolicy.h named mpol_to_str(). That function does BUILD_BUG_ON(maxlen <
>>> 64) and then calls __mpol_to_str().
>>>
>>> Modify __mpol_to_str() to store "unknown" when mpol->mode does not match
>>> any known MPOL_* constant.
>>>
>>
>> Can we be sure 'maxlen' should not be less than 64? For show_numa_map()
>> in fs/proc/task_mmu.c, it use 50 which is less than 64, is it correct?
>>
>
> Whatever the max string length is that can be stored by mpol_to_str()
> preferably rounded to the nearest power of two.
>
Do you mean: show_numa_map() in "fs/proc/task_mmu.c" also need be
'fixed', what it has done (use 50) is incorrect?
>> Can we be sure that our output contents are always less than 64 bytes?
>> Do we need BUG_ON() instead of all '-ENOSPC' in mpol_to_str()?
>>
>
> You can determine the maximum string length by looking at the
> implementation of mpol_to_str().
>
Can we be sure maximum string will be never changed in future?
>> Hmm... If assume what you said above was always correct: "we are always
>> sure 64 bytes is enough, and 'maxlen' should be never less than 64".
>>
>> It would be better to use a structure (which has a member "char buf[64]") pointer instead of 'buffer' and 'maxlen'.
>> (and also still need check 64 memory bondary and '\0' within mpol_to_str).
>>
>
> That's ridiculous, kernel developers who call mpol_to_str() aren't idiots.
>
It seems, it is not quite polite. ;-)
Hmm... for extern function, caller has duty to understand interface
precisely, but no duty to understand internal implementation.
So if callee wants caller to know about something, it needs 'express' it
through its' interface, callee can not assume that caller should
understand internal implementation.
> I think at this point it will just be best if I propose a patch and ask
> for it to be merged into the -mm tree rather than continue this thread.
>
>
Hmm... you can try to send a related patch for it, but I am not quite
sure whether can pass reviewers' checking or not.
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-17 22:53 ` David Rientjes
@ 2013-09-18 1:37 ` Chen Gang
2013-09-18 22:17 ` David Rientjes
0 siblings, 1 reply; 92+ messages in thread
From: Chen Gang @ 2013-09-18 1:37 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/18/2013 06:53 AM, David Rientjes wrote:
> On Tue, 17 Sep 2013, Chen Gang wrote:
>
>>> BUG_ON() is safe. but I still don't like it. As far as I heard, Google
>>> changes BUG_ON as nop. So, BUG_ON(mpol_to_str() < 0) breaks google.
>>> Please treat an assertion as assertion. Not any other something.
>>>
>
> Google does not disable BUG_ON(), sheesh.
>
That sounds a good news.
>> Hmm... in kernel wide, BUG_ON() is 'common' 'standard' assertion, and
>> "mm/" is a common sub-system (not architecture specific), so when we
>> use BUG_ON(), we already 'express' our 'opinion' enough to readers.
>>
>
> That's ridiculous, we're not going to panic the kernel at runtime because
> a buffer is too small. Make it a compile-time error like I suggested so
> we catch this before we even build the kernel.
>
It seems not quite polite? ;-)
BUG_ON() is widely and commonly used in kernel wide, and BUG_ON() can be
customized by any architectures, so I guess, if google really think it
is necessary, it will customize it.
If "compile-time error" will make code complex to both readers and
writers (e.g. our case), forcing "compile-time error" may still be good
enough to google, but may not be good enough for others.
So in my opinion, for our case which is a common sub-system, not an
architecture specific sub-system, better use "run-time error".
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-18 1:37 ` Chen Gang
@ 2013-09-18 22:17 ` David Rientjes
0 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-18 22:17 UTC (permalink / raw)
To: Chen Gang
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On Wed, 18 Sep 2013, Chen Gang wrote:
> BUG_ON() is widely and commonly used in kernel wide, and BUG_ON() can be
> customized by any architectures, so I guess, if google really think it
> is necessary, it will customize it.
>
> If "compile-time error" will make code complex to both readers and
> writers (e.g. our case), forcing "compile-time error" may still be good
> enough to google, but may not be good enough for others.
>
Google has nothing to do with this, it treats BUG_ON() just like 99.99% of
others do.
> So in my opinion, for our case which is a common sub-system, not an
> architecture specific sub-system, better use "run-time error".
>
That's absolutely insane. If code is not allocating enough memory for the
maximum possible length of a string to be stored by mpol_to_str(), it's a
bug in the code. We do not panic and reboot the user's machine for such a
bug. Instead, we break the build and require the broken code to be fixed.
I have told you exactly how to introduce such a compile-time error.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-05 0:24 ` [PATCH v2] " Chen Gang
@ 2013-09-25 2:58 ` David Rientjes
2013-09-25 2:58 ` David Rientjes
1 sibling, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 2:58 UTC (permalink / raw)
To: Andrew Morton
Cc: KOSAKI Motohiro, Chen Gang, Rik van Riel, linux-kernel, linux-mm
mpol_to_str() should not fail. Currently, it either fails because the
string buffer is too small or because a string hasn't been defined for a
mempolicy mode.
If a new mempolicy mode is introduced and no string is defined for it,
just warn and return "unknown".
If the buffer is too small, just truncate the string and return, the same
behavior as snprintf().
This also fixes a bug where there was no NULL-byte termination when doing
*p++ = '=' and *p++ ':' and maxlen has been reached.
Signed-off-by: David Rientjes <rientjes@google.com>
---
fs/proc/task_mmu.c | 14 ++++++-------
include/linux/mempolicy.h | 5 ++---
mm/mempolicy.c | 52 +++++++++++++++--------------------------------
3 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1385,8 +1385,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
struct mm_struct *mm = vma->vm_mm;
struct mm_walk walk = {};
struct mempolicy *pol;
- int n;
- char buffer[50];
+ char buffer[64];
+ int nid;
if (!mm)
return 0;
@@ -1402,10 +1402,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- n = mpol_to_str(buffer, sizeof(buffer), pol);
+ mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
- if (n < 0)
- return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
@@ -1458,9 +1456,9 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
if (md->writeback)
seq_printf(m, " writeback=%lu", md->writeback);
- for_each_node_state(n, N_MEMORY)
- if (md->node[n])
- seq_printf(m, " N%d=%lu", n, md->node[n]);
+ for_each_node_state(nid, N_MEMORY)
+ if (md->node[nid])
+ seq_printf(m, " N%d=%lu", nid, md->node[nid]);
out:
seq_putc(m, '\n');
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -168,7 +168,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
extern int mpol_parse_str(char *str, struct mempolicy **mpol);
#endif
-extern int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
+extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
/* Check if a vma is migratable */
static inline int vma_migratable(struct vm_area_struct *vma)
@@ -306,9 +306,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
}
#endif
-static inline int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
- return 0;
}
static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma,
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2840,62 +2840,45 @@ out:
* @maxlen: length of @buffer
* @pol: pointer to mempolicy to be formatted
*
- * Convert a mempolicy into a string.
- * Returns the number of characters in buffer (if positive)
- * or an error (negative)
+ * Convert @pol into a string. If @buffer is too short, truncate the string.
+ * Recommend a @maxlen of at least 32 for the longest mode, "interleave", the
+ * longest flag, "relative", and to display at least a few node ids.
*/
-int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
char *p = buffer;
- int l;
- nodemask_t nodes;
- unsigned short mode;
- unsigned short flags = pol ? pol->flags : 0;
-
- /*
- * Sanity check: room for longest mode, flag and some nodes
- */
- VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
+ nodemask_t nodes = NODE_MASK_NONE;
+ unsigned short mode = MPOL_DEFAULT;
+ unsigned short flags = 0;
- if (!pol || pol == &default_policy)
- mode = MPOL_DEFAULT;
- else
+ if (pol && pol != &default_policy) {
mode = pol->mode;
+ flags = pol->flags;
+ }
switch (mode) {
case MPOL_DEFAULT:
- nodes_clear(nodes);
break;
-
case MPOL_PREFERRED:
- nodes_clear(nodes);
if (flags & MPOL_F_LOCAL)
mode = MPOL_LOCAL;
else
node_set(pol->v.preferred_node, nodes);
break;
-
case MPOL_BIND:
- /* Fall through */
case MPOL_INTERLEAVE:
nodes = pol->v.nodes;
break;
-
default:
- return -EINVAL;
+ WARN_ON_ONCE(1);
+ snprintf(p, maxlen, "unknown");
+ return;
}
- l = strlen(policy_modes[mode]);
- if (buffer + maxlen < p + l + 1)
- return -ENOSPC;
-
- strcpy(p, policy_modes[mode]);
- p += l;
+ p += snprintf(p, maxlen, policy_modes[mode]);
if (flags & MPOL_MODE_FLAGS) {
- if (buffer + maxlen < p + 2)
- return -ENOSPC;
- *p++ = '=';
+ p += snprintf(p, buffer + maxlen - p, "=");
/*
* Currently, the only defined flags are mutually exclusive
@@ -2907,10 +2890,7 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
}
if (!nodes_empty(nodes)) {
- if (buffer + maxlen < p + 2)
- return -ENOSPC;
- *p++ = ':';
+ p += snprintf(p, buffer + maxlen - p, ":");
p += nodelist_scnprintf(p, buffer + maxlen - p, nodes);
}
- return p - buffer;
}
^ permalink raw reply [flat|nested] 92+ messages in thread
* [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 2:58 ` David Rientjes
0 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 2:58 UTC (permalink / raw)
To: Andrew Morton
Cc: KOSAKI Motohiro, Chen Gang, Rik van Riel, linux-kernel, linux-mm
mpol_to_str() should not fail. Currently, it either fails because the
string buffer is too small or because a string hasn't been defined for a
mempolicy mode.
If a new mempolicy mode is introduced and no string is defined for it,
just warn and return "unknown".
If the buffer is too small, just truncate the string and return, the same
behavior as snprintf().
This also fixes a bug where there was no NULL-byte termination when doing
*p++ = '=' and *p++ ':' and maxlen has been reached.
Signed-off-by: David Rientjes <rientjes@google.com>
---
fs/proc/task_mmu.c | 14 ++++++-------
include/linux/mempolicy.h | 5 ++---
mm/mempolicy.c | 52 +++++++++++++++--------------------------------
3 files changed, 24 insertions(+), 47 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1385,8 +1385,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
struct mm_struct *mm = vma->vm_mm;
struct mm_walk walk = {};
struct mempolicy *pol;
- int n;
- char buffer[50];
+ char buffer[64];
+ int nid;
if (!mm)
return 0;
@@ -1402,10 +1402,8 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
walk.mm = mm;
pol = get_vma_policy(task, vma, vma->vm_start);
- n = mpol_to_str(buffer, sizeof(buffer), pol);
+ mpol_to_str(buffer, sizeof(buffer), pol);
mpol_cond_put(pol);
- if (n < 0)
- return n;
seq_printf(m, "%08lx %s", vma->vm_start, buffer);
@@ -1458,9 +1456,9 @@ static int show_numa_map(struct seq_file *m, void *v, int is_pid)
if (md->writeback)
seq_printf(m, " writeback=%lu", md->writeback);
- for_each_node_state(n, N_MEMORY)
- if (md->node[n])
- seq_printf(m, " N%d=%lu", n, md->node[n]);
+ for_each_node_state(nid, N_MEMORY)
+ if (md->node[nid])
+ seq_printf(m, " N%d=%lu", nid, md->node[nid]);
out:
seq_putc(m, '\n');
diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -168,7 +168,7 @@ int do_migrate_pages(struct mm_struct *mm, const nodemask_t *from,
extern int mpol_parse_str(char *str, struct mempolicy **mpol);
#endif
-extern int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
+extern void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol);
/* Check if a vma is migratable */
static inline int vma_migratable(struct vm_area_struct *vma)
@@ -306,9 +306,8 @@ static inline int mpol_parse_str(char *str, struct mempolicy **mpol)
}
#endif
-static inline int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+static inline void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
- return 0;
}
static inline int mpol_misplaced(struct page *page, struct vm_area_struct *vma,
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2840,62 +2840,45 @@ out:
* @maxlen: length of @buffer
* @pol: pointer to mempolicy to be formatted
*
- * Convert a mempolicy into a string.
- * Returns the number of characters in buffer (if positive)
- * or an error (negative)
+ * Convert @pol into a string. If @buffer is too short, truncate the string.
+ * Recommend a @maxlen of at least 32 for the longest mode, "interleave", the
+ * longest flag, "relative", and to display at least a few node ids.
*/
-int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
+void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
{
char *p = buffer;
- int l;
- nodemask_t nodes;
- unsigned short mode;
- unsigned short flags = pol ? pol->flags : 0;
-
- /*
- * Sanity check: room for longest mode, flag and some nodes
- */
- VM_BUG_ON(maxlen < strlen("interleave") + strlen("relative") + 16);
+ nodemask_t nodes = NODE_MASK_NONE;
+ unsigned short mode = MPOL_DEFAULT;
+ unsigned short flags = 0;
- if (!pol || pol == &default_policy)
- mode = MPOL_DEFAULT;
- else
+ if (pol && pol != &default_policy) {
mode = pol->mode;
+ flags = pol->flags;
+ }
switch (mode) {
case MPOL_DEFAULT:
- nodes_clear(nodes);
break;
-
case MPOL_PREFERRED:
- nodes_clear(nodes);
if (flags & MPOL_F_LOCAL)
mode = MPOL_LOCAL;
else
node_set(pol->v.preferred_node, nodes);
break;
-
case MPOL_BIND:
- /* Fall through */
case MPOL_INTERLEAVE:
nodes = pol->v.nodes;
break;
-
default:
- return -EINVAL;
+ WARN_ON_ONCE(1);
+ snprintf(p, maxlen, "unknown");
+ return;
}
- l = strlen(policy_modes[mode]);
- if (buffer + maxlen < p + l + 1)
- return -ENOSPC;
-
- strcpy(p, policy_modes[mode]);
- p += l;
+ p += snprintf(p, maxlen, policy_modes[mode]);
if (flags & MPOL_MODE_FLAGS) {
- if (buffer + maxlen < p + 2)
- return -ENOSPC;
- *p++ = '=';
+ p += snprintf(p, buffer + maxlen - p, "=");
/*
* Currently, the only defined flags are mutually exclusive
@@ -2907,10 +2890,7 @@ int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
}
if (!nodes_empty(nodes)) {
- if (buffer + maxlen < p + 2)
- return -ENOSPC;
- *p++ = ':';
+ p += snprintf(p, buffer + maxlen - p, ":");
p += nodelist_scnprintf(p, buffer + maxlen - p, nodes);
}
- return p - buffer;
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 2:58 ` David Rientjes
@ 2013-09-25 3:11 ` Dave Jones
-1 siblings, 0 replies; 92+ messages in thread
From: Dave Jones @ 2013-09-25 3:11 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Tue, Sep 24, 2013 at 07:58:22PM -0700, David Rientjes wrote:
> case MPOL_BIND:
> - /* Fall through */
> case MPOL_INTERLEAVE:
> nodes = pol->v.nodes;
> break;
Any reason not to leave this ?
"missing break" is the 2nd most common thing that coverity picks up.
Most of them are false positives like the above, but the lack of annotations
in our source makes it time-consuming to pick through them all to find the
real bugs.
Dave
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 3:11 ` Dave Jones
0 siblings, 0 replies; 92+ messages in thread
From: Dave Jones @ 2013-09-25 3:11 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Tue, Sep 24, 2013 at 07:58:22PM -0700, David Rientjes wrote:
> case MPOL_BIND:
> - /* Fall through */
> case MPOL_INTERLEAVE:
> nodes = pol->v.nodes;
> break;
Any reason not to leave this ?
"missing break" is the 2nd most common thing that coverity picks up.
Most of them are false positives like the above, but the lack of annotations
in our source makes it time-consuming to pick through them all to find the
real bugs.
Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 3:11 ` Dave Jones
@ 2013-09-25 3:18 ` David Rientjes
-1 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 3:18 UTC (permalink / raw)
To: Dave Jones, Andrew Morton, KOSAKI Motohiro, Chen Gang,
Rik van Riel, linux-kernel, linux-mm
On Tue, 24 Sep 2013, Dave Jones wrote:
> > case MPOL_BIND:
> > - /* Fall through */
> > case MPOL_INTERLEAVE:
> > nodes = pol->v.nodes;
> > break;
>
> Any reason not to leave this ?
>
> "missing break" is the 2nd most common thing that coverity picks up.
> Most of them are false positives like the above, but the lack of annotations
> in our source makes it time-consuming to pick through them all to find the
> real bugs.
>
Check out things like drivers/mfd/wm5110-tables.c that do things like
switch (reg) {
case ARIZONA_SOFTWARE_RESET:
case ARIZONA_DEVICE_REVISION:
case ARIZONA_CTRL_IF_SPI_CFG_1:
case ARIZONA_CTRL_IF_I2C1_CFG_1:
case ARIZONA_CTRL_IF_I2C2_CFG_1:
case ARIZONA_CTRL_IF_I2C1_CFG_2:
case ARIZONA_CTRL_IF_I2C2_CFG_2:
...
and that file has over 1,000 case statements. Having a
/* fall through */
for all of them would be pretty annoying.
I don't remember any coding style rule about this (in fact
Documentation/CodingStyle has examples of case statements without such a
comment), I think it's just personal preference so I'll leave it to Andrew
and what he prefers.
(And if he prefers the /* fall through */ then we should ask that it be
added to checkpatch.pl since it warns about a million other things and not
this.)
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 3:18 ` David Rientjes
0 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 3:18 UTC (permalink / raw)
To: Dave Jones, Andrew Morton, KOSAKI Motohiro, Chen Gang,
Rik van Riel, linux-kernel, linux-mm
On Tue, 24 Sep 2013, Dave Jones wrote:
> > case MPOL_BIND:
> > - /* Fall through */
> > case MPOL_INTERLEAVE:
> > nodes = pol->v.nodes;
> > break;
>
> Any reason not to leave this ?
>
> "missing break" is the 2nd most common thing that coverity picks up.
> Most of them are false positives like the above, but the lack of annotations
> in our source makes it time-consuming to pick through them all to find the
> real bugs.
>
Check out things like drivers/mfd/wm5110-tables.c that do things like
switch (reg) {
case ARIZONA_SOFTWARE_RESET:
case ARIZONA_DEVICE_REVISION:
case ARIZONA_CTRL_IF_SPI_CFG_1:
case ARIZONA_CTRL_IF_I2C1_CFG_1:
case ARIZONA_CTRL_IF_I2C2_CFG_1:
case ARIZONA_CTRL_IF_I2C1_CFG_2:
case ARIZONA_CTRL_IF_I2C2_CFG_2:
...
and that file has over 1,000 case statements. Having a
/* fall through */
for all of them would be pretty annoying.
I don't remember any coding style rule about this (in fact
Documentation/CodingStyle has examples of case statements without such a
comment), I think it's just personal preference so I'll leave it to Andrew
and what he prefers.
(And if he prefers the /* fall through */ then we should ask that it be
added to checkpatch.pl since it warns about a million other things and not
this.)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 3:18 ` David Rientjes
@ 2013-09-25 3:25 ` Dave Jones
-1 siblings, 0 replies; 92+ messages in thread
From: Dave Jones @ 2013-09-25 3:25 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Tue, Sep 24, 2013 at 08:18:16PM -0700, David Rientjes wrote:
> On Tue, 24 Sep 2013, Dave Jones wrote:
>
> > > case MPOL_BIND:
> > > - /* Fall through */
> > > case MPOL_INTERLEAVE:
> > > nodes = pol->v.nodes;
> > > break;
> >
> > Any reason not to leave this ?
> >
> > "missing break" is the 2nd most common thing that coverity picks up.
> > Most of them are false positives like the above, but the lack of annotations
> > in our source makes it time-consuming to pick through them all to find the
> > real bugs.
> >
>
> Check out things like drivers/mfd/wm5110-tables.c that do things like
>
> switch (reg) {
> case ARIZONA_SOFTWARE_RESET:
> case ARIZONA_DEVICE_REVISION:
> case ARIZONA_CTRL_IF_SPI_CFG_1:
> case ARIZONA_CTRL_IF_I2C1_CFG_1:
> case ARIZONA_CTRL_IF_I2C2_CFG_1:
> case ARIZONA_CTRL_IF_I2C1_CFG_2:
> case ARIZONA_CTRL_IF_I2C2_CFG_2:
> ...
>
> and that file has over 1,000 case statements. Having a
yikes, at first I thought that was output from a code generator.
> /* fall through */
>
> for all of them would be pretty annoying.
agreed, but with that example, it seems pretty obvious (to me at least)
that the lack of break's is intentional. Where it gets trickier to
make quick judgment calls is cases like the one I mentioned above,
where there are only a few cases, and there's real code involved in
some but not all cases.
> I don't remember any coding style rule about this (in fact
> Documentation/CodingStyle has examples of case statements without such a
> comment), I think it's just personal preference so I'll leave it to Andrew
> and what he prefers.
>
> (And if he prefers the /* fall through */ then we should ask that it be
> added to checkpatch.pl since it warns about a million other things and not
> this.)
The question of course is how much gain there is in doing anything at all here.
So far, I've only seen false positives from that checker, but there are hundreds
of them to pick through, so who knows what's further down the pile.
Dave
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 3:25 ` Dave Jones
0 siblings, 0 replies; 92+ messages in thread
From: Dave Jones @ 2013-09-25 3:25 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Tue, Sep 24, 2013 at 08:18:16PM -0700, David Rientjes wrote:
> On Tue, 24 Sep 2013, Dave Jones wrote:
>
> > > case MPOL_BIND:
> > > - /* Fall through */
> > > case MPOL_INTERLEAVE:
> > > nodes = pol->v.nodes;
> > > break;
> >
> > Any reason not to leave this ?
> >
> > "missing break" is the 2nd most common thing that coverity picks up.
> > Most of them are false positives like the above, but the lack of annotations
> > in our source makes it time-consuming to pick through them all to find the
> > real bugs.
> >
>
> Check out things like drivers/mfd/wm5110-tables.c that do things like
>
> switch (reg) {
> case ARIZONA_SOFTWARE_RESET:
> case ARIZONA_DEVICE_REVISION:
> case ARIZONA_CTRL_IF_SPI_CFG_1:
> case ARIZONA_CTRL_IF_I2C1_CFG_1:
> case ARIZONA_CTRL_IF_I2C2_CFG_1:
> case ARIZONA_CTRL_IF_I2C1_CFG_2:
> case ARIZONA_CTRL_IF_I2C2_CFG_2:
> ...
>
> and that file has over 1,000 case statements. Having a
yikes, at first I thought that was output from a code generator.
> /* fall through */
>
> for all of them would be pretty annoying.
agreed, but with that example, it seems pretty obvious (to me at least)
that the lack of break's is intentional. Where it gets trickier to
make quick judgment calls is cases like the one I mentioned above,
where there are only a few cases, and there's real code involved in
some but not all cases.
> I don't remember any coding style rule about this (in fact
> Documentation/CodingStyle has examples of case statements without such a
> comment), I think it's just personal preference so I'll leave it to Andrew
> and what he prefers.
>
> (And if he prefers the /* fall through */ then we should ask that it be
> added to checkpatch.pl since it warns about a million other things and not
> this.)
The question of course is how much gain there is in doing anything at all here.
So far, I've only seen false positives from that checker, but there are hundreds
of them to pick through, so who knows what's further down the pile.
Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 3:25 ` Dave Jones
@ 2013-09-25 17:58 ` David Rientjes
-1 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 17:58 UTC (permalink / raw)
To: Dave Jones, Andrew Morton, KOSAKI Motohiro, Chen Gang,
Rik van Riel, linux-kernel, linux-mm
On Tue, 24 Sep 2013, Dave Jones wrote:
> > /* fall through */
> >
> > for all of them would be pretty annoying.
>
> agreed, but with that example, it seems pretty obvious (to me at least)
> that the lack of break's is intentional. Where it gets trickier to
> make quick judgment calls is cases like the one I mentioned above,
> where there are only a few cases, and there's real code involved in
> some but not all cases.
>
I fully agree and have code in the oom killer that has the "fall through"
comment if there's code in between the case statements, but I think things
like
case MPOL_BIND:
case MPOL_INTERLEAVE:
...
is quite easy to read. I don't feel strongly at all, though, so I'll just
leave it to Andrew's preference.
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 17:58 ` David Rientjes
0 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 17:58 UTC (permalink / raw)
To: Dave Jones, Andrew Morton, KOSAKI Motohiro, Chen Gang,
Rik van Riel, linux-kernel, linux-mm
On Tue, 24 Sep 2013, Dave Jones wrote:
> > /* fall through */
> >
> > for all of them would be pretty annoying.
>
> agreed, but with that example, it seems pretty obvious (to me at least)
> that the lack of break's is intentional. Where it gets trickier to
> make quick judgment calls is cases like the one I mentioned above,
> where there are only a few cases, and there's real code involved in
> some but not all cases.
>
I fully agree and have code in the oom killer that has the "fall through"
comment if there's code in between the case statements, but I think things
like
case MPOL_BIND:
case MPOL_INTERLEAVE:
...
is quite easy to read. I don't feel strongly at all, though, so I'll just
leave it to Andrew's preference.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 17:58 ` David Rientjes
@ 2013-09-25 21:30 ` Andrew Morton
-1 siblings, 0 replies; 92+ messages in thread
From: Andrew Morton @ 2013-09-25 21:30 UTC (permalink / raw)
To: David Rientjes
Cc: Dave Jones, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Wed, 25 Sep 2013 10:58:27 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> On Tue, 24 Sep 2013, Dave Jones wrote:
>
> > > /* fall through */
> > >
> > > for all of them would be pretty annoying.
> >
> > agreed, but with that example, it seems pretty obvious (to me at least)
> > that the lack of break's is intentional. Where it gets trickier to
> > make quick judgment calls is cases like the one I mentioned above,
> > where there are only a few cases, and there's real code involved in
> > some but not all cases.
> >
>
> I fully agree and have code in the oom killer that has the "fall through"
> comment if there's code in between the case statements, but I think things
> like
>
> case MPOL_BIND:
> case MPOL_INTERLEAVE:
> ...
>
> is quite easy to read. I don't feel strongly at all, though, so I'll just
> leave it to Andrew's preference.
I've never even thought about it, but that won't prevent me from
pretending otherwise! How about:
This:
case WIBBLE:
something();
something_else();
case WOBBLE:
needs a /* fall through */ comment (because it *looks* like a mistake),
whereas
case WIBBLE:
case WOBBLE:
does not?
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 21:30 ` Andrew Morton
0 siblings, 0 replies; 92+ messages in thread
From: Andrew Morton @ 2013-09-25 21:30 UTC (permalink / raw)
To: David Rientjes
Cc: Dave Jones, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Wed, 25 Sep 2013 10:58:27 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> On Tue, 24 Sep 2013, Dave Jones wrote:
>
> > > /* fall through */
> > >
> > > for all of them would be pretty annoying.
> >
> > agreed, but with that example, it seems pretty obvious (to me at least)
> > that the lack of break's is intentional. Where it gets trickier to
> > make quick judgment calls is cases like the one I mentioned above,
> > where there are only a few cases, and there's real code involved in
> > some but not all cases.
> >
>
> I fully agree and have code in the oom killer that has the "fall through"
> comment if there's code in between the case statements, but I think things
> like
>
> case MPOL_BIND:
> case MPOL_INTERLEAVE:
> ...
>
> is quite easy to read. I don't feel strongly at all, though, so I'll just
> leave it to Andrew's preference.
I've never even thought about it, but that won't prevent me from
pretending otherwise! How about:
This:
case WIBBLE:
something();
something_else();
case WOBBLE:
needs a /* fall through */ comment (because it *looks* like a mistake),
whereas
case WIBBLE:
case WOBBLE:
does not?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
2013-09-25 21:30 ` Andrew Morton
@ 2013-09-25 22:06 ` David Rientjes
-1 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 22:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Dave Jones, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Wed, 25 Sep 2013, Andrew Morton wrote:
> > I fully agree and have code in the oom killer that has the "fall through"
> > comment if there's code in between the case statements, but I think things
> > like
> >
> > case MPOL_BIND:
> > case MPOL_INTERLEAVE:
> > ...
> >
> > is quite easy to read. I don't feel strongly at all, though, so I'll just
> > leave it to Andrew's preference.
>
> I've never even thought about it, but that won't prevent me from
> pretending otherwise! How about:
>
> This:
>
> case WIBBLE:
> something();
> something_else();
> case WOBBLE:
>
> needs a /* fall through */ comment (because it *looks* like a mistake),
> whereas
>
> case WIBBLE:
> case WOBBLE:
>
> does not?
>
The switch-case examples given in Documentation/CodingStyle agree with
that.
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [patch] mm, mempolicy: make mpol_to_str robust and always succeed
@ 2013-09-25 22:06 ` David Rientjes
0 siblings, 0 replies; 92+ messages in thread
From: David Rientjes @ 2013-09-25 22:06 UTC (permalink / raw)
To: Andrew Morton
Cc: Dave Jones, KOSAKI Motohiro, Chen Gang, Rik van Riel,
linux-kernel, linux-mm
On Wed, 25 Sep 2013, Andrew Morton wrote:
> > I fully agree and have code in the oom killer that has the "fall through"
> > comment if there's code in between the case statements, but I think things
> > like
> >
> > case MPOL_BIND:
> > case MPOL_INTERLEAVE:
> > ...
> >
> > is quite easy to read. I don't feel strongly at all, though, so I'll just
> > leave it to Andrew's preference.
>
> I've never even thought about it, but that won't prevent me from
> pretending otherwise! How about:
>
> This:
>
> case WIBBLE:
> something();
> something_else();
> case WOBBLE:
>
> needs a /* fall through */ comment (because it *looks* like a mistake),
> whereas
>
> case WIBBLE:
> case WOBBLE:
>
> does not?
>
The switch-case examples given in Documentation/CodingStyle agree with
that.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
2013-09-23 21:46 ` David Rientjes
@ 2013-09-24 2:28 ` Chen Gang
0 siblings, 0 replies; 92+ messages in thread
From: Chen Gang @ 2013-09-24 2:28 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
On 09/24/2013 05:46 AM, David Rientjes wrote:
> On Thu, 19 Sep 2013, Chen,Gang( e??a??) wrote:
>
>> Please search BUG_ON() in kernel wide source code, we can know whether
>> it is commonly used or not.
>>
>> Please search BUG in arch/ sub-system, we can know which architectures
>> customize BUG/BUG_ON.
>>
>> After do the 2 things, In my opinion, we can treat BUG/BUG_ON() is common
>> implementation, and most of architectures uses the default one.
>>
>> Please check again, thanks.
>>
>
> BUG_ON() is used for fatal conditions where continuing could potentially
> be harmful. Obviously it is commonly used in a kernel. That doesn't mean
> we BUG_ON() when a string hasn't been defined for a mempolicy mode.
> mpol_to_str() is not critical.
>
> It is not a fatal condition, and nothing you say is going to convince
> anybody on this thread that it's a fatal condition.
>
If mpol_to_str() fail, the buffer passed to next seq_printf() may cause
memory over flow.
So in current implementation, if mpol_to_str() fails, it may cause
critical issue ("it's a fatal condition").
My original fix is "check and return if fail", but related members think
it shouldn't fail, so use BUG_ON(): "when fails, means OS is continuing
blindly, and next, may cause direct critical issue".
>>> That's absolutely insane. If code is not allocating enough memory for the
>>> maximum possible length of a string to be stored by mpol_to_str(), it's a
>>> bug in the code. We do not panic and reboot the user's machine for such a
>>> bug. Instead, we break the build and require the broken code to be fixed.
>>>
>>
>> Please say in polite.
>>
>
> You want a polite response when you're insisting that we declare absolute
> failure, BUG_ON(), stop, and reboot the kernel because a mempolicy mode
> isn't defined as a string in mpol_to_str()? That sounds like an impolite
> response to the user, so see my politeness to you as coming from the users
> of the systems you just crashed.
>
Hmm... Except God, we (everyone, include real users) only can discuss
judge, and check things and actions based on the proves, but has no
right to discuss, judge and check persons.
When it is just discussing (not get a conclusion), it is impolite to
make a conclusion forcefully by oneself with his/her own feelings.
And when we really get a conclusion, we need use the words which only
can express the result clearly (e.g. correct, incorrect) to make a
conclusion, not use words also contents his/her own feelings.
> This is a compile-time problem, not run-time.
>
Hmm... I am not quite familiar with the details, but at least for me,
what you said is acceptable (at least, we can try).
Current mpol_to_str() interface leads all readers have to treat it as
run-time problem, not as compile-time problem.
So in my opinion, if really try the compile-time fix, the interface need
be changed: "need use struct (have a member buf[64];) pointer instead of
'buffer' and 'maxlen' parameters".
>> Can you be sure, the "maxlen == 50" in "fs/proc/task_mmu()", must be a bug??
>>
>
> I asked you to figure out the longest string possible to be stored by
> mpol_to_str(). There's nothing mysterious about that function. It's
> deterministic. If you really can't figure out the value this should be,
> then you shouldn't be touching mpol_to_str().
>
At present, it seems easy to get longest string, so you can try, don't
need me; in future, I don't know whether it will be changed, maybe you
know (need give a length long enough for future using).
I don't plan to touch mpol_to_str(), in my opinion, just treat it as
run-time problem is still acceptable: it is clear enough for readers and
writers.
Hmm... at last, I still welcome you to try to send your compile-time fix
patch for it (although I am not quite sure whether it will be acceptable
by other members).
Thanks.
--
Chen Gang
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
[not found] <20130919003142.B72EC1840296@intranet.asianux.com>
@ 2013-09-23 21:46 ` David Rientjes
2013-09-24 2:28 ` Chen Gang
0 siblings, 1 reply; 92+ messages in thread
From: David Rientjes @ 2013-09-23 21:46 UTC (permalink / raw)
To: Chen,Gang( 陈刚)
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2016 bytes --]
On Thu, 19 Sep 2013, Chen,Gang( e??a??) wrote:
> PleaseA searchA BUG_ON()A inA kernelA wideA sourceA code,A weA canA knowA whether
> itA isA commonlyA usedA orA not.
>
> PleaseA searchA BUGA inA arch/A sub-system,A weA canA knowA whichA architectures
> customizeA BUG/BUG_ON.
>
> AfterA doA theA 2A things,A InA myA opinion,A weA canA treatA BUG/BUG_ON()A isA common
> implementation,A andA mostA ofA architecturesA usesA theA defaultA one.
>
> PleaseA checkA again,A thanks.
>
BUG_ON() is used for fatal conditions where continuing could potentially
be harmful. Obviously it is commonly used in a kernel. That doesn't mean
we BUG_ON() when a string hasn't been defined for a mempolicy mode.
mpol_to_str() is not critical.
It is not a fatal condition, and nothing you say is going to convince
anybody on this thread that it's a fatal condition.
> >A That'sA absolutelyA insane.A A IfA codeA isA notA allocatingA enoughA memoryA forA theA
> >A maximumA possibleA lengthA ofA aA stringA toA beA storedA byA mpol_to_str(),A it'sA aA
> >A bugA inA theA code.A A WeA doA notA panicA andA rebootA theA user'sA machineA forA suchA aA
> >A bug.A A Instead,A weA breakA theA buildA andA requireA theA brokenA codeA toA beA fixed.
> >A
>
> PleaseA sayA inA polite.
>
You want a polite response when you're insisting that we declare absolute
failure, BUG_ON(), stop, and reboot the kernel because a mempolicy mode
isn't defined as a string in mpol_to_str()? That sounds like an impolite
response to the user, so see my politeness to you as coming from the users
of the systems you just crashed.
This is a compile-time problem, not run-time.
> CanA youA beA sure,A theA "maxlenA ==A 50"A inA "fs/proc/task_mmu()",A mustA beA aA bug??
>
I asked you to figure out the longest string possible to be stored by
mpol_to_str(). There's nothing mysterious about that function. It's
deterministic. If you really can't figure out the value this should be,
then you shouldn't be touching mpol_to_str().
^ permalink raw reply [flat|nested] 92+ messages in thread
* Re: [PATCH v2] mm/shmem.c: check the return value of mpol_to_str()
@ 2013-09-19 0:31 Chen,Gang( 陈刚)
0 siblings, 0 replies; 92+ messages in thread
From: Chen,Gang( 陈刚) @ 2013-09-19 0:31 UTC (permalink / raw)
To: David Rientjes
Cc: KOSAKI Motohiro, KOSAKI Motohiro, riel, hughd, xemul, Wanpeng Li,
Cyrill Gorcunov, linux-mm, Andrew Morton
[-- Attachment #1: Type: text/html, Size: 5029 bytes --]
^ permalink raw reply [flat|nested] 92+ messages in thread
end of thread, other threads:[~2013-09-25 22:06 UTC | newest]
Thread overview: 92+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-20 3:56 [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str() Chen Gang
2013-08-20 3:56 ` Chen Gang
2013-08-20 3:57 ` [PATCH 1/3] mm/mempolicy.c: still fill buffer as full as possible when buffer space is not enough in mpol_to_str() Chen Gang
2013-08-20 3:57 ` Chen Gang
2013-08-20 3:58 ` [PATCH 2/3] fs/proc/task_mmu.c: check the return value of mpol_to_str() Chen Gang
2013-08-20 3:58 ` Chen Gang
2013-08-20 3:59 ` [PATCH 3/3] mm/shmem.c: " Chen Gang
2013-08-20 3:59 ` Chen Gang
2013-08-20 5:30 ` [PATCH 0/3] mm: mempolicy: the failure processing about mpol_to_str() Cyrill Gorcunov
2013-08-20 5:30 ` Cyrill Gorcunov
2013-08-20 5:41 ` Chen Gang
2013-08-20 5:41 ` Chen Gang
2013-08-20 6:47 ` Cyrill Gorcunov
2013-08-20 6:47 ` Cyrill Gorcunov
2013-08-20 7:48 ` Chen Gang
2013-08-20 7:48 ` Chen Gang
2013-08-20 7:51 ` Chen Gang
2013-08-20 7:51 ` Chen Gang
2013-08-20 8:09 ` Chen Gang
2013-08-20 8:09 ` Chen Gang
2013-08-20 8:13 ` Chen Gang F T
2013-08-20 8:13 ` Chen Gang F T
2013-08-20 8:20 ` Chen Gang
2013-08-20 8:20 ` Chen Gang
2013-08-20 8:25 ` Cyrill Gorcunov
2013-08-20 8:25 ` Cyrill Gorcunov
2013-08-20 8:31 ` Chen Gang
2013-08-20 8:31 ` Chen Gang
2013-08-21 2:21 ` [PATCH 0/3] mm: shmem: check the return value of mpol_to_str() Chen Gang
2013-08-21 2:21 ` Chen Gang
2013-08-21 2:22 ` [PATCH 1/3] fs/proc/task_mmu.c: " Chen Gang
2013-08-21 2:22 ` Chen Gang
2013-08-21 2:23 ` [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value Chen Gang
2013-08-21 2:23 ` Chen Gang
2013-08-21 2:24 ` [PATCH 3/3] mm/shmem.c: check the return value of mpol_to_str() Chen Gang
2013-08-21 2:24 ` Chen Gang
2013-08-21 22:03 ` [PATCH 2/3] mm/shmem.c: let shmem_show_mpol() return value Andrew Morton
2013-08-21 22:03 ` Andrew Morton
2013-08-22 0:52 ` Chen Gang
2013-08-22 0:52 ` Chen Gang
2013-08-22 1:04 ` [PATCH] mm/shmem.c: check the return value of mpol_to_str() Chen Gang
2013-08-22 1:04 ` Chen Gang
2013-09-03 5:32 ` Chen Gang
2013-09-03 5:32 ` Chen Gang
2013-09-05 0:24 ` [PATCH v2] " Chen Gang
2013-09-09 20:30 ` David Rientjes
2013-09-10 0:47 ` Chen Gang
2013-09-10 6:43 ` David Rientjes
2013-09-10 7:01 ` Chen Gang
2013-09-12 0:33 ` David Rientjes
2013-09-12 2:19 ` KOSAKI Motohiro
2013-09-12 3:13 ` Chen Gang
2013-09-13 21:12 ` David Rientjes
2013-09-14 2:51 ` KOSAKI Motohiro
2013-09-16 3:27 ` Chen Gang
2013-09-16 20:13 ` David Rientjes
2013-09-17 0:45 ` Chen Gang
2013-09-17 22:51 ` David Rientjes
2013-09-18 1:20 ` Chen Gang
2013-09-12 3:02 ` Chen Gang
2013-09-12 18:19 ` KOSAKI Motohiro
2013-09-13 2:23 ` Chen Gang
2013-09-13 16:50 ` KOSAKI Motohiro
2013-09-16 2:55 ` Chen Gang
2013-09-16 16:16 ` KOSAKI Motohiro
2013-09-17 1:10 ` Chen Gang
2013-09-17 22:53 ` David Rientjes
2013-09-18 1:37 ` Chen Gang
2013-09-18 22:17 ` David Rientjes
2013-09-13 21:14 ` David Rientjes
2013-09-16 3:17 ` Chen Gang
2013-09-25 2:58 ` [patch] mm, mempolicy: make mpol_to_str robust and always succeed David Rientjes
2013-09-25 2:58 ` David Rientjes
2013-09-25 3:11 ` Dave Jones
2013-09-25 3:11 ` Dave Jones
2013-09-25 3:18 ` David Rientjes
2013-09-25 3:18 ` David Rientjes
2013-09-25 3:25 ` Dave Jones
2013-09-25 3:25 ` Dave Jones
2013-09-25 17:58 ` David Rientjes
2013-09-25 17:58 ` David Rientjes
2013-09-25 21:30 ` Andrew Morton
2013-09-25 21:30 ` Andrew Morton
2013-09-25 22:06 ` David Rientjes
2013-09-25 22:06 ` David Rientjes
2013-08-21 5:31 ` [PATCH 0/3] mm: shmem: check the return value of mpol_to_str() Cyrill Gorcunov
2013-08-21 5:31 ` Cyrill Gorcunov
2013-08-21 5:48 ` Chen Gang
2013-08-21 5:48 ` Chen Gang
2013-09-19 0:31 [PATCH v2] mm/shmem.c: " Chen,Gang( 陈刚)
[not found] <20130919003142.B72EC1840296@intranet.asianux.com>
2013-09-23 21:46 ` David Rientjes
2013-09-24 2:28 ` Chen Gang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.