Re: [PATCH 1/2] quota: reorder flags in quota state

[Konstantin Khlebnikov <khlebnikov@yandex-team.ru>]

On 12.02.2015 18:05, Jan Kara wrote:
> On Thu 12-02-15 12:36:44, Konstantin Khlebnikov wrote:
>> Flags in struct quota_state keep flags for each quota type and
>> some common flags. This patch reorders typed flags:
>>
>> Before:
>>
>> 0 USRQUOTA DQUOT_USAGE_ENABLED
>> 1 USRQUOTA DQUOT_LIMITS_ENABLED
>> 2 USRQUOTA DQUOT_SUSPENDED
>> 3 GRPQUOTA DQUOT_USAGE_ENABLED
>> 4 GRPQUOTA DQUOT_LIMITS_ENABLED
>> 5 GRPQUOTA DQUOT_SUSPENDED
>> 6          DQUOT_QUOTA_SYS_FILE
>> 7          DQUOT_NEGATIVE_USAGE
>>
>> After:
>>
>> 0 USRQUOTA DQUOT_USAGE_ENABLED
>> 1 GRPQUOTA DQUOT_USAGE_ENABLED
>> 2 USRQUOTA DQUOT_LIMITS_ENABLED
>> 3 GRPQUOTA DQUOT_LIMITS_ENABLED
>> 4 USRQUOTA DQUOT_SUSPENDED
>> 5 GRPQUOTA DQUOT_SUSPENDED
>> 6          DQUOT_QUOTA_SYS_FILE
>> 7          DQUOT_NEGATIVE_USAGE
>>
>> Now we can get bitmap of all enabled/suspended quota types without loop.
>> For example suspended: (flags / DQUOT_SUSPENDED) & ((1 << MAXQUOTAS) - 1).
>>
>> add/remove: 0/1 grow/shrink: 3/11 up/down: 56/-215 (-159)
>> function                                     old     new   delta
>> __dquot_initialize                           423     447     +24
>> dquot_transfer                               181     197     +16
>> dquot_alloc_inode                            286     302     +16
>> dquot_reclaim_space_nodirty                  316     313      -3
>> dquot_claim_space_nodirty                    314     311      -3
>> dquot_resume                                 286     281      -5
>> dquot_free_inode                             332     324      -8
>> __dquot_alloc_space                          500     492      -8
>> dquot_disable                               1944    1929     -15
>> dquot_quota_enable                           252     236     -16
>> __dquot_free_space                           750     734     -16
>> dquot_writeback_dquots                       625     608     -17
>> __dquot_transfer                            1186    1154     -32
>> dquot_quota_sync                             299     261     -38
>> dquot_active.isra                             54       -     -54
>    Two minor comments below. Otherwise the patch looks good.
>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> ---
>>   include/linux/quota.h    |   18 ++++++++++++------
>>   include/linux/quotaops.h |   10 ++--------
>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/linux/quota.h b/include/linux/quota.h
>> index d534e8e..205b4f7 100644
>> --- a/include/linux/quota.h
>> +++ b/include/linux/quota.h
>> @@ -398,9 +398,9 @@ enum {
>>   						 * memory to turn them on */
>>   	_DQUOT_STATE_FLAGS
>>   };
>    Please explain how the flags are exactly laid out in a comment in
> include/linux/quota.h at the enum definition above.
>
>> -#define DQUOT_USAGE_ENABLED	(1 << _DQUOT_USAGE_ENABLED)
>> -#define DQUOT_LIMITS_ENABLED	(1 << _DQUOT_LIMITS_ENABLED)
>> -#define DQUOT_SUSPENDED		(1 << _DQUOT_SUSPENDED)
>> +#define DQUOT_USAGE_ENABLED	(1 << _DQUOT_USAGE_ENABLED * MAXQUOTAS)
>> +#define DQUOT_LIMITS_ENABLED	(1 << _DQUOT_LIMITS_ENABLED * MAXQUOTAS)
>> +#define DQUOT_SUSPENDED		(1 << _DQUOT_SUSPENDED * MAXQUOTAS)
>>   #define DQUOT_STATE_FLAGS	(DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED | \
>>   				 DQUOT_SUSPENDED)
>>   /* Other quota flags */
>> @@ -414,15 +414,21 @@ enum {
>>   						 */
>>   #define DQUOT_NEGATIVE_USAGE	(1 << (DQUOT_STATE_LAST + 1))
>>   					       /* Allow negative quota usage */
>> -
>>   static inline unsigned int dquot_state_flag(unsigned int flags, int type)
>>   {
>> -	return flags << _DQUOT_STATE_FLAGS * type;
>> +	return flags << type;
>>   }
>>
>>   static inline unsigned int dquot_generic_flag(unsigned int flags, int type)
>>   {
>> -	return (flags >> _DQUOT_STATE_FLAGS * type) & DQUOT_STATE_FLAGS;
>> +	return (flags >> type) & DQUOT_STATE_FLAGS;
>> +}
>> +
>> +/* Bitmap of quota types where flag is set in flags */
>> +static __always_inline unsigned dquot_state_types(unsigned flags, unsigned flag)
>    Why do you have __always_inline here? Just use inline if the function
> works standalone (which seems to be the case here).

It works better if "flag" is constant -- dividing turns into bit shift.
And I'm not sure how BUILD_BUG_ON* work for non-constant expressions.

>
>> +{
>> +	BUILD_BUG_ON_NOT_POWER_OF_2(flag);
>> +	return (flags / flag) & ((1 << MAXQUOTAS) - 1);
>>   }
>>
>>   #ifdef CONFIG_QUOTA_NETLINK_INTERFACE
>
> 								Honza
>


-- 
Konstantin