[PATCH] drivers/base/memory: simplify outputting of valid_zones_show()

[PATCH] drivers/base/memory: simplify outputting of valid_zones_show()

[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.

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);
 	}
 
 	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

Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()

[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.

-- 
Cheers,

David / dhildenb

Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()

[Shiyang Ruan]

在 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.

Re: [PATCH] drivers/base/memory: simplify outputting of valid_zones_show()

[David Hildenbrand]

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

[PATCH v2] drivers/base/memory: simplify outputting of valid_zones_show()

[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>
---
 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

Re: [PATCH v2] drivers/base/memory: simplify outputting of valid_zones_show()

[David Hildenbrand]

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

[PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()

[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;
 }
-- 
2.43.0

Re: [PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()

[Shiyang Ruan]

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;
>   }

Re: [PATCH v3] drivers/base/memory: simplify outputting of valid_zones_show()

[David Hildenbrand]

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