Message ID | 1282144088.2068.16.camel@castor.rsk |
---|---|
State | Superseded, archived |
Headers | show |
Richard Kennedy wrote: > Reorder structure ext4_inode_info to remove 16 bytes of alignment > padding on 64 bit builds. This shrinks its size from 904 to 888 bytes > with (CONFIG_EXT4_S_XATTR=y && CONFIG_QUOTA=n). > > This will allow this structure to use one fewer cache lines. > > Also change type of i_delalloc_reserved_flag to bool to better reflect > its usage. > > compiled & tested on x86_64 Looks good to me; there is still a bit of padding at the end: qsize_t i_reserved_quota; /* 864 8 */ struct list_head i_completed_io_list; /* 872 16 */ spinlock_t i_completed_io_lock; /* 888 4 */ /* XXX 4 bytes hole, try to pack */ /* --- cacheline 14 boundary (896 bytes) --- */ ext4_io_end_t * cur_aio_dio; /* 896 8 */ tid_t i_sync_tid; /* 904 4 */ tid_t i_datasync_tid; /* 908 4 */ but it wouldn't save a cacheline anyway... -Eric > Signed-off-by: Richard Kennedy <richard@rsk.demon.co.uk> > --- > patch against 2.6.36-rc1 > > Early testing shows more consistent results ( i.e. smaller standard > deviations) on my AMD X2 setup, but I shall re-run these tests to check > if this is repeatable. > > regards > Richard > > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 889ec9d..b63a8e3 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -750,10 +750,11 @@ struct ext4_inode_info { > * near to their parent directory's inode. > */ > ext4_group_t i_block_group; > - unsigned long i_state_flags; /* Dynamic state flags */ > - unsigned long i_flags; > > ext4_lblk_t i_dir_start_lookup; > + > + unsigned long i_state_flags; /* Dynamic state flags */ > + unsigned long i_flags; > #ifdef CONFIG_EXT4_FS_XATTR > /* > * Extended attributes can be read independently of the main file > @@ -816,13 +817,14 @@ struct ext4_inode_info { > unsigned int i_reserved_data_blocks; > unsigned int i_reserved_meta_blocks; > unsigned int i_allocated_meta_blocks; > - unsigned short i_delalloc_reserved_flag; > - sector_t i_da_metadata_calc_last_lblock; > - int i_da_metadata_calc_len; > + bool i_delalloc_reserved_flag; > > /* on-disk additional length */ > __u16 i_extra_isize; > > + sector_t i_da_metadata_calc_last_lblock; > + int i_da_metadata_calc_len; > + > spinlock_t i_block_reservation_lock; > #ifdef CONFIG_QUOTA > /* quota space reservation, managed internally by quota code */ > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2010-08-20 at 15:11 -0500, Eric Sandeen wrote: > Richard Kennedy wrote: > > Reorder structure ext4_inode_info to remove 16 bytes of alignment > > padding on 64 bit builds. This shrinks its size from 904 to 888 bytes > > with (CONFIG_EXT4_S_XATTR=y && CONFIG_QUOTA=n). > > > > This will allow this structure to use one fewer cache lines. > > > > Also change type of i_delalloc_reserved_flag to bool to better reflect > > its usage. > > > > compiled & tested on x86_64 > > > Looks good to me; there is still a bit of padding at the end: > > qsize_t i_reserved_quota; /* 864 8 */ > struct list_head i_completed_io_list; /* 872 16 */ > spinlock_t i_completed_io_lock; /* 888 4 */ > > /* XXX 4 bytes hole, try to pack */ > > /* --- cacheline 14 boundary (896 bytes) --- */ > ext4_io_end_t * cur_aio_dio; /* 896 8 */ > tid_t i_sync_tid; /* 904 4 */ > tid_t i_datasync_tid; /* 908 4 */ > > > but it wouldn't save a cacheline anyway... > > -Eric > Thanks Eric, yes there is padding but we don't have anything to fill the hole with :( If it becomes important to save space, i_state_flags seem to be unused in ext4 so we could possibly drop that. With my patch applied, under slub I'm getting one more object per slab in the ext4_inode_cache [2.6.35] 17 objects of 960 bytes -- 4 pages per slab [+patch] 18 objects of 896 bytes -- 4 pages per slab So I guess that is where most of the write test performance improvement came from. regards Richard -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Richard Kennedy wrote: > On Fri, 2010-08-20 at 15:11 -0500, Eric Sandeen wrote: >> Richard Kennedy wrote: >>> Reorder structure ext4_inode_info to remove 16 bytes of alignment >>> padding on 64 bit builds. This shrinks its size from 904 to 888 bytes >>> with (CONFIG_EXT4_S_XATTR=y && CONFIG_QUOTA=n). >>> >>> This will allow this structure to use one fewer cache lines. >>> >>> Also change type of i_delalloc_reserved_flag to bool to better reflect >>> its usage. >>> >>> compiled & tested on x86_64 >> >> Looks good to me; there is still a bit of padding at the end: >> >> qsize_t i_reserved_quota; /* 864 8 */ >> struct list_head i_completed_io_list; /* 872 16 */ >> spinlock_t i_completed_io_lock; /* 888 4 */ >> >> /* XXX 4 bytes hole, try to pack */ >> >> /* --- cacheline 14 boundary (896 bytes) --- */ >> ext4_io_end_t * cur_aio_dio; /* 896 8 */ >> tid_t i_sync_tid; /* 904 4 */ >> tid_t i_datasync_tid; /* 908 4 */ >> >> >> but it wouldn't save a cacheline anyway... >> >> -Eric >> > Thanks Eric, yes there is padding but we don't have anything to fill the > hole with :( > > If it becomes important to save space, i_state_flags seem to be unused > in ext4 so we could possibly drop that. Actually it is used, it's just expertly hidden in: EXT4_INODE_BIT_FNS(state, state_flags): #define EXT4_INODE_BIT_FNS(name, field) \ static inline int ext4_test_inode_##name(struct inode *inode, int bit) \ { \ return test_bit(bit, &EXT4_I(inode)->i_##field); \ } \ etc.. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 889ec9d..b63a8e3 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -750,10 +750,11 @@ struct ext4_inode_info { * near to their parent directory's inode. */ ext4_group_t i_block_group; - unsigned long i_state_flags; /* Dynamic state flags */ - unsigned long i_flags; ext4_lblk_t i_dir_start_lookup; + + unsigned long i_state_flags; /* Dynamic state flags */ + unsigned long i_flags; #ifdef CONFIG_EXT4_FS_XATTR /* * Extended attributes can be read independently of the main file @@ -816,13 +817,14 @@ struct ext4_inode_info { unsigned int i_reserved_data_blocks; unsigned int i_reserved_meta_blocks; unsigned int i_allocated_meta_blocks; - unsigned short i_delalloc_reserved_flag; - sector_t i_da_metadata_calc_last_lblock; - int i_da_metadata_calc_len; + bool i_delalloc_reserved_flag; /* on-disk additional length */ __u16 i_extra_isize; + sector_t i_da_metadata_calc_last_lblock; + int i_da_metadata_calc_len; + spinlock_t i_block_reservation_lock; #ifdef CONFIG_QUOTA /* quota space reservation, managed internally by quota code */
Reorder structure ext4_inode_info to remove 16 bytes of alignment padding on 64 bit builds. This shrinks its size from 904 to 888 bytes with (CONFIG_EXT4_S_XATTR=y && CONFIG_QUOTA=n). This will allow this structure to use one fewer cache lines. Also change type of i_delalloc_reserved_flag to bool to better reflect its usage. compiled & tested on x86_64 Signed-off-by: Richard Kennedy <richard@rsk.demon.co.uk> --- patch against 2.6.36-rc1 Early testing shows more consistent results ( i.e. smaller standard deviations) on my AMD X2 setup, but I shall re-run these tests to check if this is repeatable. regards Richard -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html