[PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Vasily Averin]

'element' and 'handle' are union in struct zram_table_entry.

Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")
Signed-off-by: Vasily Averin <vasily.averin@linux.dev>
---
 drivers/block/zram/zram_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9ac3d4e51d26..f4d342d11b81 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1318,12 +1318,10 @@ static int zram_read_from_zspool(struct zram *zram, struct page *page,
 
 	handle = zram_get_handle(zram, index);
 	if (!handle || zram_test_flag(zram, index, ZRAM_SAME)) {
-		unsigned long value;
 		void *mem;
 
-		value = handle ? zram_get_element(zram, index) : 0;
 		mem = kmap_atomic(page);
-		zram_fill_page(mem, PAGE_SIZE, value);
+		zram_fill_page(mem, PAGE_SIZE, handle);
 		kunmap_atomic(mem);
 		return 0;
 	}
-- 
2.34.1

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Vasily Averin]

On 11/6/23 22:55, Vasily Averin wrote:
> 'element' and 'handle' are union in struct zram_table_entry.

struct zram_table_entry {
        union {
                unsigned long handle;
                unsigned long element;
        };

I do not understand the sense of this union.
From my POV it just makes it harder to check the code because an reviewer doesn't
expect that the zram element can't be used together.
Can I remove this union at all and replace zram_get/set_element calls by zram_get/set_handle instead?

Thank you,
	Vasily Averin

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Sergey Senozhatsky]

On (23/11/06 23:03), Vasily Averin wrote:
> On 11/6/23 22:55, Vasily Averin wrote:
> > 'element' and 'handle' are union in struct zram_table_entry.
> 
> struct zram_table_entry {
>         union {
>                 unsigned long handle;
>                 unsigned long element;
>         };
> 
> I do not understand the sense of this union.
> From my POV it just makes it harder to check the code because an reviewer doesn't
> expect that the zram element can't be used together.
> Can I remove this union at all and replace zram_get/set_element calls by zram_get/set_handle instead?

I guess it sort of helps API-wise to distinguish zram_handle (allocated
zsmalloc object handle) and zram_element (same-filled entry).

I'll leave it to Minchan to decide.

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Sergey Senozhatsky]

On (23/11/06 22:55), Vasily Averin wrote:
> 
> 'element' and 'handle' are union in struct zram_table_entry.
> 
> Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")

Sorry, what exactly does it fix?

[..]
> @@ -1318,12 +1318,10 @@ static int zram_read_from_zspool(struct zram *zram, struct page *page,
>  
>  	handle = zram_get_handle(zram, index);
>  	if (!handle || zram_test_flag(zram, index, ZRAM_SAME)) {
> -		unsigned long value;
>  		void *mem;
>  
> -		value = handle ? zram_get_element(zram, index) : 0;
>  		mem = kmap_atomic(page);
> -		zram_fill_page(mem, PAGE_SIZE, value);
> +		zram_fill_page(mem, PAGE_SIZE, handle);
>  		kunmap_atomic(mem);
>  		return 0;
>  	}

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Vasily Averin]

On 11/8/23 05:49, Sergey Senozhatsky wrote:
> On (23/11/06 22:55), Vasily Averin wrote:
>>
>> 'element' and 'handle' are union in struct zram_table_entry.
>>
>> Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")
> 
> Sorry, what exactly does it fix?

It removes unneeded call of zram_get_element() and unneeded variable 'value'.
zram_get_element() == zram_get_handle(), they both access the same field of the same struct zram_table_entry,
no need to read it 2nd time. 
'value' variable is not required, 'handle' can be used instead.

I hope this explain why element/handle union should be removed: it confuses reviewers.

> [..]
>> @@ -1318,12 +1318,10 @@ static int zram_read_from_zspool(struct zram *zram, struct page *page,
>>  
>>  	handle = zram_get_handle(zram, index);
>>  	if (!handle || zram_test_flag(zram, index, ZRAM_SAME)) {
>> -		unsigned long value;
>>  		void *mem;
>>  
>> -		value = handle ? zram_get_element(zram, index) : 0;
>>  		mem = kmap_atomic(page);
>> -		zram_fill_page(mem, PAGE_SIZE, value);
>> +		zram_fill_page(mem, PAGE_SIZE, handle);
>>  		kunmap_atomic(mem);
>>  		return 0;
>>  	}

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Sergey Senozhatsky]

On (23/11/08 06:16), Vasily Averin wrote:
> On 11/8/23 05:49, Sergey Senozhatsky wrote:
> > On (23/11/06 22:55), Vasily Averin wrote:
> >>
> >> 'element' and 'handle' are union in struct zram_table_entry.
> >>
> >> Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")
> > 
> > Sorry, what exactly does it fix?
> 
> It removes unneeded call of zram_get_element() and unneeded variable 'value'.

Yes, what the patch does is pretty clear. It doesn't *fix* anything per se.

> zram_get_element() == zram_get_handle(), they both access the same field of the same struct zram_table_entry,
> no need to read it 2nd time. 
> 'value' variable is not required, 'handle' can be used instead.
> 
> I hope this explain why element/handle union should be removed: it confuses reviewers.

I do not agree with "union should be removed" part.

In this particular case - using handle as the page pattern (element)
is in fact quite confusing. The visual separation of `handle` and `element`
is helpful.

Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()

[Vasily Averin]

On 11/8/23 06:32, Sergey Senozhatsky wrote:
> On (23/11/08 06:16), Vasily Averin wrote:
>> On 11/8/23 05:49, Sergey Senozhatsky wrote:
>>> On (23/11/06 22:55), Vasily Averin wrote:
>>>>
>>>> 'element' and 'handle' are union in struct zram_table_entry.
>>>>
>>>> Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")
>>>
>>> Sorry, what exactly does it fix?
>>
>> It removes unneeded call of zram_get_element() and unneeded variable 'value'.
> 
> Yes, what the patch does is pretty clear. It doesn't *fix* anything per se.

Ok, I'm sorry for miscommunication.
I'm agree, it is just minor cleanup.
"Fixes:" tag just here was pointed to the patch added this problem.
Perhaps it was better to specify something like "Introduced-by:" tag instead.

>> zram_get_element() == zram_get_handle(), they both access the same field of the same struct zram_table_entry,
>> no need to read it 2nd time. 
>> 'value' variable is not required, 'handle' can be used instead.
>>
>> I hope this explain why element/handle union should be removed: it confuses reviewers.
> 
> I do not agree with "union should be removed" part.
> 
> In this particular case - using handle as the page pattern (element)
> is in fact quite confusing. The visual separation of `handle` and `element`
> is helpful.

It's at your discretion, you know better.

Thank you,
	Vasily Averin