* Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:09 [PATCH] drivers/base/memory: simplify outputting of valid_zones_show() Shiyang Ruan
@ 2025-01-07 10:15 ` David Hildenbrand
2025-01-07 10:28 ` Shiyang Ruan
2025-01-07 10:39 ` [PATCH v2] " Shiyang Ruan
2025-01-08 1:52 ` [PATCH v3] " Shiyang Ruan
2 siblings, 1 reply; 9+ messages in thread
From: David Hildenbrand @ 2025-01-07 10:15 UTC (permalink / raw)
To: Shiyang Ruan, linux-mm, osalvador, rafael
On 07.01.25 11:09, Shiyang Ruan wrote:
> No need to specific position at the first writing to the buf because the
> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
>
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
> drivers/base/memory.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 67858eeb92ed..d77a83c9af39 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -469,19 +469,17 @@ static ssize_t valid_zones_show(struct device *dev,
> default_zone = mem->zone;
> if (!default_zone)
> return sysfs_emit(buf, "%s\n", "none");
> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
> - goto out;
> + return sysfs_emit(buf, "%s\n", default_zone->name);
We can go one step further and do:
return sysfs_emit(buf, "%s\n",
mem->zone ? mem->zone->name : "none");
Avoiding setting/checking default_zone.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:15 ` David Hildenbrand
@ 2025-01-07 10:28 ` Shiyang Ruan
2025-01-07 10:33 ` David Hildenbrand
0 siblings, 1 reply; 9+ messages in thread
From: Shiyang Ruan @ 2025-01-07 10:28 UTC (permalink / raw)
To: David Hildenbrand; +Cc: linux-mm, osalvador, rafael
在 2025/1/7 18:15, David Hildenbrand 写道:
> On 07.01.25 11:09, Shiyang Ruan wrote:
>> No need to specific position at the first writing to the buf because the
>> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>> ---
>> drivers/base/memory.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 67858eeb92ed..d77a83c9af39 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -469,19 +469,17 @@ static ssize_t valid_zones_show(struct device *dev,
>> default_zone = mem->zone;
>> if (!default_zone)
>> return sysfs_emit(buf, "%s\n", "none");
>> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
>> - goto out;
>> + return sysfs_emit(buf, "%s\n", default_zone->name);
>
> We can go one step further and do:
>
> return sysfs_emit(buf, "%s\n",
> mem->zone ? mem->zone->name : "none");
>
> Avoiding setting/checking default_zone.
I have thought about it but it would cause the line over long. Now that
it doesn't matter, I'll make the chage as you suggested and send a new
version.
Thanks!
--
Ruan.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:28 ` Shiyang Ruan
@ 2025-01-07 10:33 ` David Hildenbrand
0 siblings, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2025-01-07 10:33 UTC (permalink / raw)
To: Shiyang Ruan; +Cc: linux-mm, osalvador, rafael
On 07.01.25 11:28, Shiyang Ruan wrote:
>
>
> 在 2025/1/7 18:15, David Hildenbrand 写道:
>> On 07.01.25 11:09, Shiyang Ruan wrote:
>>> No need to specific position at the first writing to the buf because the
>>> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
>>>
>>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>>> ---
>>> drivers/base/memory.c | 6 ++----
>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>>> index 67858eeb92ed..d77a83c9af39 100644
>>> --- a/drivers/base/memory.c
>>> +++ b/drivers/base/memory.c
>>> @@ -469,19 +469,17 @@ static ssize_t valid_zones_show(struct device *dev,
>>> default_zone = mem->zone;
>>> if (!default_zone)
>>> return sysfs_emit(buf, "%s\n", "none");
>>> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
>>> - goto out;
>>> + return sysfs_emit(buf, "%s\n", default_zone->name);
>>
>> We can go one step further and do:
>>
>> return sysfs_emit(buf, "%s\n",
>> mem->zone ? mem->zone->name : "none");
>>
>> Avoiding setting/checking default_zone.
>
> I have thought about it but it would cause the line over long.
I think it fits even into 80 chars just fine if you split it across two
lines like I suggested.
But yeah, a single line might be fine as well.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:09 [PATCH] drivers/base/memory: simplify outputting of valid_zones_show() Shiyang Ruan
2025-01-07 10:15 ` David Hildenbrand
@ 2025-01-07 10:39 ` Shiyang Ruan
2025-01-07 10:52 ` David Hildenbrand
2025-01-08 1:52 ` [PATCH v3] " Shiyang Ruan
2 siblings, 1 reply; 9+ messages in thread
From: Shiyang Ruan @ 2025-01-07 10:39 UTC (permalink / raw)
To: linux-mm, david, osalvador, rafael
No need to specific position at the first writing to the buf because the
@len is always 0 at this time. Use sysfs_emit() instead to simplify it.
Also avoid setting/checking default_zone with a conditional operator.
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
---
drivers/base/memory.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 67858eeb92ed..975fdbbff264 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -466,22 +466,18 @@ static ssize_t valid_zones_show(struct device *dev,
* If !mem->zone, the memory block spans multiple zones and
* cannot get offlined.
*/
- default_zone = mem->zone;
- if (!default_zone)
- return sysfs_emit(buf, "%s\n", "none");
- len += sysfs_emit_at(buf, len, "%s", default_zone->name);
- goto out;
+ return sysfs_emit(buf, "%s\n",
+ mem->zone ? mem->zone->name : "none");
}
default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, group,
start_pfn, nr_pages);
- len += sysfs_emit_at(buf, len, "%s", default_zone->name);
+ len += sysfs_emit(buf, "%s", default_zone->name);
len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
MMOP_ONLINE_KERNEL, default_zone);
len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
MMOP_ONLINE_MOVABLE, default_zone);
-out:
len += sysfs_emit_at(buf, len, "\n");
return len;
}
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:39 ` [PATCH v2] " Shiyang Ruan
@ 2025-01-07 10:52 ` David Hildenbrand
0 siblings, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2025-01-07 10:52 UTC (permalink / raw)
To: Shiyang Ruan, linux-mm, osalvador, rafael
On 07.01.25 11:39, Shiyang Ruan wrote:
> No need to specific position at the first writing to the buf because the
> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
> Also avoid setting/checking default_zone with a conditional operator.
>
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> ---
> drivers/base/memory.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 67858eeb92ed..975fdbbff264 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -466,22 +466,18 @@ static ssize_t valid_zones_show(struct device *dev,
> * If !mem->zone, the memory block spans multiple zones and
> * cannot get offlined.
> */
> - default_zone = mem->zone;
> - if (!default_zone)
> - return sysfs_emit(buf, "%s\n", "none");
> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
> - goto out;
> + return sysfs_emit(buf, "%s\n",
> + mem->zone ? mem->zone->name : "none");
> }
>
> default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, group,
> start_pfn, nr_pages);
>
> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
> + len += sysfs_emit(buf, "%s", default_zone->name);
Sorry for realizing it now:
you can turn that into
len = sysfs_emit(buf, "%s", default_zone->name);
And stop initializing len to 0.
Acked-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-07 10:09 [PATCH] drivers/base/memory: simplify outputting of valid_zones_show() Shiyang Ruan
2025-01-07 10:15 ` David Hildenbrand
2025-01-07 10:39 ` [PATCH v2] " Shiyang Ruan
@ 2025-01-08 1:52 ` Shiyang Ruan
2025-01-22 10:14 ` Shiyang Ruan
2 siblings, 1 reply; 9+ messages in thread
From: Shiyang Ruan @ 2025-01-08 1:52 UTC (permalink / raw)
To: linux-mm, david, osalvador, rafael
No need to specific position at the first writing to the buf because the
@len is always 0 at this time. Use sysfs_emit() instead to simplify it.
Also avoid setting/checking default_zone with a conditional operator.
Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 67858eeb92ed..92e6bc6eb21e 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -455,7 +455,7 @@ static ssize_t valid_zones_show(struct device *dev,
struct memory_group *group = mem->group;
struct zone *default_zone;
int nid = mem->nid;
- int len = 0;
+ int len;
/*
* Check the existing zone. Make sure that we do that only on the
@@ -466,22 +466,18 @@ static ssize_t valid_zones_show(struct device *dev,
* If !mem->zone, the memory block spans multiple zones and
* cannot get offlined.
*/
- default_zone = mem->zone;
- if (!default_zone)
- return sysfs_emit(buf, "%s\n", "none");
- len += sysfs_emit_at(buf, len, "%s", default_zone->name);
- goto out;
+ return sysfs_emit(buf, "%s\n",
+ mem->zone ? mem->zone->name : "none");
}
default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, group,
start_pfn, nr_pages);
- len += sysfs_emit_at(buf, len, "%s", default_zone->name);
+ len = sysfs_emit(buf, "%s", default_zone->name);
len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
MMOP_ONLINE_KERNEL, default_zone);
len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
MMOP_ONLINE_MOVABLE, default_zone);
-out:
len += sysfs_emit_at(buf, len, "\n");
return len;
}
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-08 1:52 ` [PATCH v3] " Shiyang Ruan
@ 2025-01-22 10:14 ` Shiyang Ruan
2025-01-22 11:04 ` David Hildenbrand
0 siblings, 1 reply; 9+ messages in thread
From: Shiyang Ruan @ 2025-01-22 10:14 UTC (permalink / raw)
To: linux-mm, david, osalvador, rafael
Hi,
ping~
Any other comments? Thanks.
在 2025/1/8 9:52, Shiyang Ruan 写道:
> No need to specific position at the first writing to the buf because the
> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
> Also avoid setting/checking default_zone with a conditional operator.
>
> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
> drivers/base/memory.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 67858eeb92ed..92e6bc6eb21e 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -455,7 +455,7 @@ static ssize_t valid_zones_show(struct device *dev,
> struct memory_group *group = mem->group;
> struct zone *default_zone;
> int nid = mem->nid;
> - int len = 0;
> + int len;
>
> /*
> * Check the existing zone. Make sure that we do that only on the
> @@ -466,22 +466,18 @@ static ssize_t valid_zones_show(struct device *dev,
> * If !mem->zone, the memory block spans multiple zones and
> * cannot get offlined.
> */
> - default_zone = mem->zone;
> - if (!default_zone)
> - return sysfs_emit(buf, "%s\n", "none");
> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
> - goto out;
> + return sysfs_emit(buf, "%s\n",
> + mem->zone ? mem->zone->name : "none");
> }
>
> default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, group,
> start_pfn, nr_pages);
>
> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
> + len = sysfs_emit(buf, "%s", default_zone->name);
> len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
> MMOP_ONLINE_KERNEL, default_zone);
> len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
> MMOP_ONLINE_MOVABLE, default_zone);
> -out:
> len += sysfs_emit_at(buf, len, "\n");
> return len;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()
2025-01-22 10:14 ` Shiyang Ruan
@ 2025-01-22 11:04 ` David Hildenbrand
0 siblings, 0 replies; 9+ messages in thread
From: David Hildenbrand @ 2025-01-22 11:04 UTC (permalink / raw)
To: Shiyang Ruan, linux-mm, osalvador, rafael, akpm
On 22.01.25 11:14, Shiyang Ruan wrote:
> Hi,
>
> ping~
> Any other comments? Thanks.
>
Hi,
you should always CC Andrew.
@Andrew:
https://lkml.kernel.org/r/20250108015223.1522887-1-ruansy.fnst@fujitsu.com
>
> 在 2025/1/8 9:52, Shiyang Ruan 写道:
>> No need to specific position at the first writing to the buf because the
>> @len is always 0 at this time. Use sysfs_emit() instead to simplify it.
>> Also avoid setting/checking default_zone with a conditional operator.
>>
>> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> ---
>> drivers/base/memory.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 67858eeb92ed..92e6bc6eb21e 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -455,7 +455,7 @@ static ssize_t valid_zones_show(struct device *dev,
>> struct memory_group *group = mem->group;
>> struct zone *default_zone;
>> int nid = mem->nid;
>> - int len = 0;
>> + int len;
>>
>> /*
>> * Check the existing zone. Make sure that we do that only on the
>> @@ -466,22 +466,18 @@ static ssize_t valid_zones_show(struct device *dev,
>> * If !mem->zone, the memory block spans multiple zones and
>> * cannot get offlined.
>> */
>> - default_zone = mem->zone;
>> - if (!default_zone)
>> - return sysfs_emit(buf, "%s\n", "none");
>> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
>> - goto out;
>> + return sysfs_emit(buf, "%s\n",
>> + mem->zone ? mem->zone->name : "none");
>> }
>>
>> default_zone = zone_for_pfn_range(MMOP_ONLINE, nid, group,
>> start_pfn, nr_pages);
>>
>> - len += sysfs_emit_at(buf, len, "%s", default_zone->name);
>> + len = sysfs_emit(buf, "%s", default_zone->name);
>> len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
>> MMOP_ONLINE_KERNEL, default_zone);
>> len += print_allowed_zone(buf, len, nid, group, start_pfn, nr_pages,
>> MMOP_ONLINE_MOVABLE, default_zone);
>> -out:
>> len += sysfs_emit_at(buf, len, "\n");
>> return len;
>> }
>
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 9+ messages in thread