| Message ID | 20231212110844.19698-5-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block atomic writes | expand |
On Tue 12-12-23 11:08:32, John Garry wrote: > Currently all bits are being used in fmode_t. > > To allow for further expansion, increase from unsigned int to unsigned > long. > > Since the dma-buf driver prints the file->f_mode member, change the print > as necessary to deal with the larger size. > > Signed-off-by: John Garry <john.g.garry@oracle.com> Uh, Al has more experience with fmode_t changes so I'd defer final decision to him but to me this seems dangerous. Firstly, this breaks packing of struct file on 64-bit architectures and struct file is highly optimized for cache efficiency (see the comment before the struct definition). Secondly this will probably generate warnings on 32-bit architectures as there sizeof(unsigned long) == sizeof(unsigned int) and so your new flags won't fit anyway? Honza > --- > drivers/dma-buf/dma-buf.c | 2 +- > include/linux/types.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 21916bba77d5..a5227ae3d637 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > > > spin_lock(&buf_obj->name_lock); > - seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > + seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > file_count(buf_obj->file), > diff --git a/include/linux/types.h b/include/linux/types.h > index 253168bb3fe1..49c754fde1d6 100644 > --- a/include/linux/types.h > +++ b/include/linux/types.h > @@ -153,7 +153,7 @@ typedef u32 dma_addr_t; > > typedef unsigned int __bitwise gfp_t; > typedef unsigned int __bitwise slab_flags_t; > -typedef unsigned int __bitwise fmode_t; > +typedef unsigned long __bitwise fmode_t; > > #ifdef CONFIG_PHYS_ADDR_T_64BIT > typedef u64 phys_addr_t; > -- > 2.35.3 >
On Tue, Dec 12, 2023 at 11:08:32AM +0000, John Garry wrote: > Currently all bits are being used in fmode_t. > > To allow for further expansion, increase from unsigned int to unsigned > long. > > Since the dma-buf driver prints the file->f_mode member, change the print > as necessary to deal with the larger size. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > drivers/dma-buf/dma-buf.c | 2 +- > include/linux/types.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 21916bba77d5..a5227ae3d637 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > > > spin_lock(&buf_obj->name_lock); > - seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", > + seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n", > buf_obj->size, > buf_obj->file->f_flags, buf_obj->file->f_mode, > file_count(buf_obj->file), > diff --git a/include/linux/types.h b/include/linux/types.h > index 253168bb3fe1..49c754fde1d6 100644 > --- a/include/linux/types.h > +++ b/include/linux/types.h > @@ -153,7 +153,7 @@ typedef u32 dma_addr_t; > > typedef unsigned int __bitwise gfp_t; > typedef unsigned int __bitwise slab_flags_t; > -typedef unsigned int __bitwise fmode_t; > +typedef unsigned long __bitwise fmode_t; As Jan said, that's likely a bad idea. There's a bunch of places that assume fmode_t is 32bit. So not really a change we want to make if we can avoid it.
On 13/12/2023 11:20, Jan Kara wrote: >> To allow for further expansion, increase from unsigned int to unsigned >> long. >> >> Since the dma-buf driver prints the file->f_mode member, change the print >> as necessary to deal with the larger size. >> >> Signed-off-by: John Garry<john.g.garry@oracle.com> > Uh, Al has more experience with fmode_t changes so I'd defer final decision > to him but to me this seems dangerous. Ack > Firstly, this breaks packing of > struct file on 64-bit architectures and struct file is highly optimized for > cache efficiency (see the comment before the struct definition). From pahole, I think that we still fit on the same 64B cacheline (x86_64), but some padding has been added. Before: struct file { union { struct llist_node f_llist; /* 0 8 */ struct callback_head f_rcuhead __attribute__((__aligned__(8))); /* 0 16 */ unsigned int f_iocb_flags; /* 0 4 */ } __attribute__((__aligned__(8))); /* 0 16 */ spinlock_t f_lock; /* 16 4 */ fmode_t f_mode; /* 20 4 */ atomic_long_t f_count; /* 24 8 */ struct mutex f_pos_lock; /* 32 32 */ /* --- cacheline 1 boundary (64 bytes) --- */ After: struct file { union { struct llist_node f_llist /* 0 8 */ struct callback_head f_rcuhead __attribute__((__aligned__(8))); /* 0 16 */ unsigned int f_iocb_flags; /* 0 4 */ } __attribute__((__aligned__(8))); /* 0 16 */ spinlock_t f_lock; /* 16 4 */ /* XXX 4 bytes hole, try to pack */ fmode_t f_mode; /* 24 8 */ atomic_long_t f_count; /* 32 8 */ struct mutex f_pos_lock; /* 40 32 */ /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */ > Secondly > this will probably generate warnings on 32-bit architectures as there > sizeof(unsigned long) == sizeof(unsigned int) and so your new flags won't > fit anyway? Right, it would then need to be unsigned long long. Or add another 32b member for extended modes. There were no i386 build warnings. Thanks, John
On 13/12/2023 13:02, Christian Brauner wrote: > On Tue, Dec 12, 2023 at 11:08:32AM +0000, John Garry wrote: >> Currently all bits are being used in fmode_t. >> >> To allow for further expansion, increase from unsigned int to unsigned >> long. >> >> Since the dma-buf driver prints the file->f_mode member, change the print >> as necessary to deal with the larger size. >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> drivers/dma-buf/dma-buf.c | 2 +- >> include/linux/types.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 21916bba77d5..a5227ae3d637 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) >> >> >> spin_lock(&buf_obj->name_lock); >> - seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", >> + seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n", >> buf_obj->size, >> buf_obj->file->f_flags, buf_obj->file->f_mode, >> file_count(buf_obj->file), >> diff --git a/include/linux/types.h b/include/linux/types.h >> index 253168bb3fe1..49c754fde1d6 100644 >> --- a/include/linux/types.h >> +++ b/include/linux/types.h >> @@ -153,7 +153,7 @@ typedef u32 dma_addr_t; >> >> typedef unsigned int __bitwise gfp_t; >> typedef unsigned int __bitwise slab_flags_t; >> -typedef unsigned int __bitwise fmode_t; >> +typedef unsigned long __bitwise fmode_t; > > As Jan said, that's likely a bad idea. There's a bunch of places that > assume fmode_t is 32bit. So not really a change we want to make if we > can avoid it. ok, understood. Some strictly unnecessary bits in f_mode could be recycled (if there were any), but this issue will prob come up again. Could it be considered to add an extended fmode_t member in struct file? Thanks, John
On Wed, Dec 13, 2023 at 02:02:31PM +0100, Christian Brauner wrote: > > typedef unsigned int __bitwise gfp_t; > > typedef unsigned int __bitwise slab_flags_t; > > -typedef unsigned int __bitwise fmode_t; > > +typedef unsigned long __bitwise fmode_t; > > As Jan said, that's likely a bad idea. There's a bunch of places that > assume fmode_t is 32bit. So not really a change we want to make if we > can avoid it. Oh well, let me dust of my series to move the fairly static flags out of it. But even without that do we even need to increase it? There's still quite a lot of space after FMODE_EXEC for example.
> But even without that do we even need to increase it? There's > still quite a lot of space after FMODE_EXEC for example. Right, I can use the space after FMODE_EXEC, which came free after removal of FMODE_NDELAY, FMODE_EXCL, and FMODE_WRITE_IOCTL in v6.5. Thanks, John
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 21916bba77d5..a5227ae3d637 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) spin_lock(&buf_obj->name_lock); - seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", + seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n", buf_obj->size, buf_obj->file->f_flags, buf_obj->file->f_mode, file_count(buf_obj->file), diff --git a/include/linux/types.h b/include/linux/types.h index 253168bb3fe1..49c754fde1d6 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -153,7 +153,7 @@ typedef u32 dma_addr_t; typedef unsigned int __bitwise gfp_t; typedef unsigned int __bitwise slab_flags_t; -typedef unsigned int __bitwise fmode_t; +typedef unsigned long __bitwise fmode_t; #ifdef CONFIG_PHYS_ADDR_T_64BIT typedef u64 phys_addr_t;
Currently all bits are being used in fmode_t. To allow for further expansion, increase from unsigned int to unsigned long. Since the dma-buf driver prints the file->f_mode member, change the print as necessary to deal with the larger size. Signed-off-by: John Garry <john.g.garry@oracle.com> --- drivers/dma-buf/dma-buf.c | 2 +- include/linux/types.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)