diff mbox series

[1/7] drm: Do not round to megabytes for greater than 1MiB sizes in fdinfo stats

Message ID 20230927133843.247957-2-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series fdinfo memory stats | expand

Commit Message

Tvrtko Ursulin Sept. 27, 2023, 1:38 p.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It is better not to lose precision and not revert to 1 MiB size
granularity for every size greater than 1 MiB.

Sizes in KiB should not be so troublesome to read (and in fact machine
parsing is I expect the norm here), they align with other api like
/proc/meminfo, and they allow writing tests for the interface without
having to embed drm.ko implementation knowledge into them. (Like knowing
that minimum buffer size one can use for successful verification has to be
1MiB aligned, and on top account for any pre-existing memory utilisation
outside of driver's control.)

But probably even more importantly I think that it is just better to show
the accurate sizes and not arbitrary lose precision for a little bit of a
stretched use case of eyeballing fdinfo text directly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Adrián Larumbe <adrian.larumbe@collabora.com>
Cc: steven.price@arm.com
---
 drivers/gpu/drm/drm_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Price Sept. 27, 2023, 1:48 p.m. UTC | #1 
On 27/09/2023 14:38, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> It is better not to lose precision and not revert to 1 MiB size
> granularity for every size greater than 1 MiB.
> 
> Sizes in KiB should not be so troublesome to read (and in fact machine
> parsing is I expect the norm here), they align with other api like
> /proc/meminfo, and they allow writing tests for the interface without
> having to embed drm.ko implementation knowledge into them. (Like knowing
> that minimum buffer size one can use for successful verification has to be
> 1MiB aligned, and on top account for any pre-existing memory utilisation
> outside of driver's control.)
> 
> But probably even more importantly I think that it is just better to show
> the accurate sizes and not arbitrary lose precision for a little bit of a
> stretched use case of eyeballing fdinfo text directly.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Adrián Larumbe <adrian.larumbe@collabora.com>
> Cc: steven.price@arm.com

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/drm_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index e692770ef6d3..ecb5038009e7 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -913,7 +913,7 @@ static void print_size(struct drm_printer *p, const char *stat,
>  	unsigned u;
>  
>  	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> -		if (sz < SZ_1K)
> +		if (sz == 0 || !IS_ALIGNED(sz, SZ_1K))
>  			break;
>  		sz = div_u64(sz, SZ_1K);
>  	}
Tvrtko Ursulin Sept. 28, 2023, 12:47 p.m. UTC | #2 
On 27/09/2023 14:48, Steven Price wrote:
> On 27/09/2023 14:38, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> It is better not to lose precision and not revert to 1 MiB size
>> granularity for every size greater than 1 MiB.
>>
>> Sizes in KiB should not be so troublesome to read (and in fact machine
>> parsing is I expect the norm here), they align with other api like
>> /proc/meminfo, and they allow writing tests for the interface without
>> having to embed drm.ko implementation knowledge into them. (Like knowing
>> that minimum buffer size one can use for successful verification has to be
>> 1MiB aligned, and on top account for any pre-existing memory utilisation
>> outside of driver's control.)
>>
>> But probably even more importantly I think that it is just better to show
>> the accurate sizes and not arbitrary lose precision for a little bit of a
>> stretched use case of eyeballing fdinfo text directly.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Adrián Larumbe <adrian.larumbe@collabora.com>
>> Cc: steven.price@arm.com
> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Thanks! Rob? Can we have your blessing? Could you live with KiBs? :)

Regards,

Tvrtko

>> ---
>>   drivers/gpu/drm/drm_file.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index e692770ef6d3..ecb5038009e7 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -913,7 +913,7 @@ static void print_size(struct drm_printer *p, const char *stat,
>>   	unsigned u;
>>   
>>   	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
>> -		if (sz < SZ_1K)
>> +		if (sz == 0 || !IS_ALIGNED(sz, SZ_1K))
>>   			break;
>>   		sz = div_u64(sz, SZ_1K);
>>   	}
>
Tvrtko Ursulin Oct. 26, 2023, 2:43 p.m. UTC | #3 
On 28/09/2023 13:47, Tvrtko Ursulin wrote:
> 
> On 27/09/2023 14:48, Steven Price wrote:
>> On 27/09/2023 14:38, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> It is better not to lose precision and not revert to 1 MiB size
>>> granularity for every size greater than 1 MiB.
>>>
>>> Sizes in KiB should not be so troublesome to read (and in fact machine
>>> parsing is I expect the norm here), they align with other api like
>>> /proc/meminfo, and they allow writing tests for the interface without
>>> having to embed drm.ko implementation knowledge into them. (Like knowing
>>> that minimum buffer size one can use for successful verification has 
>>> to be
>>> 1MiB aligned, and on top account for any pre-existing memory utilisation
>>> outside of driver's control.)
>>>
>>> But probably even more importantly I think that it is just better to 
>>> show
>>> the accurate sizes and not arbitrary lose precision for a little bit 
>>> of a
>>> stretched use case of eyeballing fdinfo text directly.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Cc: Adrián Larumbe <adrian.larumbe@collabora.com>
>>> Cc: steven.price@arm.com
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Thanks! Rob? Can we have your blessing? Could you live with KiBs? :)

Acked received on #dri-devel:

[12:15] <tursulin> robclark: ping on https://lists.freedesktop.org/archives/dri-devel/2023-September/424905.html - can you live with it or object?
[14:41] <robclark> tursulin: a-b

Adding the drm-misc maintainers with an ask to merge please.

Regards,

Tvrtko

> 
> Regards,
> 
> Tvrtko
> 
>>> ---
>>>   drivers/gpu/drm/drm_file.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index e692770ef6d3..ecb5038009e7 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -913,7 +913,7 @@ static void print_size(struct drm_printer *p, 
>>> const char *stat,
>>>       unsigned u;
>>>       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
>>> -        if (sz < SZ_1K)
>>> +        if (sz == 0 || !IS_ALIGNED(sz, SZ_1K))
>>>               break;
>>>           sz = div_u64(sz, SZ_1K);
>>>       }
>>
Tvrtko Ursulin Nov. 2, 2023, 9:50 a.m. UTC | #4 
On 26/10/2023 15:43, Tvrtko Ursulin wrote:
> 
> On 28/09/2023 13:47, Tvrtko Ursulin wrote:
>>
>> On 27/09/2023 14:48, Steven Price wrote:
>>> On 27/09/2023 14:38, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> It is better not to lose precision and not revert to 1 MiB size
>>>> granularity for every size greater than 1 MiB.
>>>>
>>>> Sizes in KiB should not be so troublesome to read (and in fact machine
>>>> parsing is I expect the norm here), they align with other api like
>>>> /proc/meminfo, and they allow writing tests for the interface without
>>>> having to embed drm.ko implementation knowledge into them. (Like 
>>>> knowing
>>>> that minimum buffer size one can use for successful verification has 
>>>> to be
>>>> 1MiB aligned, and on top account for any pre-existing memory 
>>>> utilisation
>>>> outside of driver's control.)
>>>>
>>>> But probably even more importantly I think that it is just better to 
>>>> show
>>>> the accurate sizes and not arbitrary lose precision for a little bit 
>>>> of a
>>>> stretched use case of eyeballing fdinfo text directly.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>> Cc: Adrián Larumbe <adrian.larumbe@collabora.com>
>>>> Cc: steven.price@arm.com
>>>
>>> Reviewed-by: Steven Price <steven.price@arm.com>
>>
>> Thanks! Rob? Can we have your blessing? Could you live with KiBs? :)
> 
> Acked received on #dri-devel:
> 
> [12:15] <tursulin> robclark: ping on 
> https://lists.freedesktop.org/archives/dri-devel/2023-September/424905.html - can you live with it or object?
> [14:41] <robclark> tursulin: a-b
> 
> Adding the drm-misc maintainers with an ask to merge please.

Ping please - just this one patch to merge to drm-misc-next if possible.

Regards,

Tvrtko
Maxime Ripard Nov. 6, 2023, 10:45 a.m. UTC | #5 
On Wed, 27 Sep 2023 14:38:37 +0100, Tvrtko Ursulin wrote:
> It is better not to lose precision and not revert to 1 MiB size
> granularity for every size greater than 1 MiB.
> 
> Sizes in KiB should not be so troublesome to read (and in fact machine
> parsing is I expect the norm here), they align with other api like
> /proc/meminfo, and they allow writing tests for the interface without
> having to embed drm.ko implementation knowledge into them. (Like knowing
> that minimum buffer size one can use for successful verification has to be
> 1MiB aligned, and on top account for any pre-existing memory utilisation
> outside of driver's control.)
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e692770ef6d3..ecb5038009e7 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -913,7 +913,7 @@  static void print_size(struct drm_printer *p, const char *stat,
 	unsigned u;
 
 	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
-		if (sz < SZ_1K)
+		if (sz == 0 || !IS_ALIGNED(sz, SZ_1K))
 			break;
 		sz = div_u64(sz, SZ_1K);
 	}