From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khlebnikov Subject: Re: [PATCH 1/2] quota: reorder flags in quota state Date: Thu, 12 Feb 2015 18:46:31 +0300 Message-ID: <54DCCAD7.8040509@yandex-team.ru> References: <20150212093644.6055.17019.stgit@buzz> <20150212150540.GC12905@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Jan Kara Return-path: In-Reply-To: <20150212150540.GC12905@quack.suse.cz> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org 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 >> --- >> 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