[PATCH chao/erofs-dev 1/2] staging: erofs: separate into init_once / always

[Chao Yu]

Hi Xiang,

On 2018/10/31 10:11, Gao Xiang wrote:
> Hi Chao,
> 
> On 2018/10/31 10:04, Chao Yu wrote:
>> On 2018/10/19 14:41, Gao Xiang wrote:
>>> `z_erofs_vle_workgroup' is heavily generated in the decompression,
>>> for example, it resets 32 bytes redundantly for 64-bit platforms
>>> even through Z_EROFS_VLE_INLINE_PAGEVECS + Z_EROFS_CLUSTER_MAX_PAGES,
>>> default 4, pages are stored in `z_erofs_vle_workgroup'.
>>>
>>> As an another example, `struct mutex' takes 72 bytes for our kirin
>>> 64-bit platforms, it's unnecessary to be reseted at first and
>>> be initialized each time.
>>>
>>> Let's avoid filling all `z_erofs_vle_workgroup' with 0 at first
>>> since most fields are reinitialized to meaningful values later,
>>> and pagevec is no need to initialized at all.
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>> ---
>>>  drivers/staging/erofs/unzip_vle.c | 34 +++++++++++++++++++++++++++++-----
>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
>>> index 79d3ba62b298..7aa26818054a 100644
>>> --- a/drivers/staging/erofs/unzip_vle.c
>>> +++ b/drivers/staging/erofs/unzip_vle.c
>>> @@ -42,12 +42,38 @@ static inline int init_unzip_workqueue(void)
>>>  	return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
>>>  }
>>>  
>>> +static void init_once(void *ptr)
>> How about rename it to init_vle_workgroup() for better readability?
> The name convension is borrowed from inode_init_always/inode_init_once of fs/inode.c...

Yes, I guess it is. :)

> since it is pure static, I don't add the prefix(such as z_erofs_vle_workgroup or etc.. too long...).
> 
> p.s. These days I think "vle_" is redundant... It could be renamed at a proper time...

Okay, naming is not a big deal, please feal free to do that.

Thanks,

> 
> 
>>
>>> +{
>>> +	struct z_erofs_vle_workgroup *grp = ptr;
>>> +	struct z_erofs_vle_work *const work =
>>> +		z_erofs_vle_grab_primary_work(grp);
>>> +	unsigned int i;
>>> +
>>> +	mutex_init(&work->lock);
>>> +	work->nr_pages = 0;
>>> +	work->vcnt = 0;
>>> +	for (i = 0; i < Z_EROFS_CLUSTER_MAX_PAGES; ++i)
>>> +		grp->compressed_pages[i] = NULL;
>>> +}
>>> +
>>> +static void init_always(struct z_erofs_vle_workgroup *grp)
>> Ditto, maybe reset_vle_workgroup().
>>
>> Otherwise, it looks good to me.
>>
>> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
> 
> Thanks for the review :)
> 
> Thanks,
> Gao Xiang
> 
> 
>>
>> Thanks,
>>
> 
> .
>