include/rdma/restrack.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Group some variables based on their sizes to reduce hole and avoid padding.
On x86_64, this shrinks the size from 136 to 128 bytes.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Using pahole
Before:
======
struct rdma_restrack_entry {
bool valid; /* 0 1 */
u8 no_track:1; /* 1: 0 1 */
/* XXX 7 bits hole, try to pack */
/* XXX 2 bytes hole, try to pack */
struct kref kref; /* 4 4 */
struct completion comp; /* 8 96 */
/* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
struct task_struct * task; /* 104 8 */
const char * kern_name; /* 112 8 */
enum rdma_restrack_type type; /* 120 4 */
bool user; /* 124 1 */
/* XXX 3 bytes hole, try to pack */
/* --- cacheline 2 boundary (128 bytes) --- */
u32 id; /* 128 4 */
/* size: 136, cachelines: 3, members: 9 */
/* sum members: 126, holes: 2, sum holes: 5 */
/* sum bitfield members: 1 bits, bit holes: 1, sum bit holes: 7 bits */
/* padding: 4 */
/* last cacheline: 8 bytes */
};
After:
=====
struct rdma_restrack_entry {
bool valid; /* 0 1 */
u8 no_track:1; /* 1: 0 1 */
/* XXX 7 bits hole, try to pack */
bool user; /* 2 1 */
/* XXX 1 byte hole, try to pack */
struct kref kref; /* 4 4 */
struct completion comp; /* 8 96 */
/* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
struct task_struct * task; /* 104 8 */
const char * kern_name; /* 112 8 */
enum rdma_restrack_type type; /* 120 4 */
u32 id; /* 124 4 */
/* size: 128, cachelines: 2, members: 9 */
/* sum members: 126, holes: 1, sum holes: 1 */
/* sum bitfield members: 1 bits, bit holes: 1, sum bit holes: 7 bits */
};
---
include/rdma/restrack.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
index 8b7c46daeb07..da53fefe6f9e 100644
--- a/include/rdma/restrack.h
+++ b/include/rdma/restrack.h
@@ -80,6 +80,10 @@ struct rdma_restrack_entry {
* query stage.
*/
u8 no_track : 1;
+ /**
+ * @user: user resource
+ */
+ bool user;
/*
* @kref: Protect destroy of the resource
*/
@@ -106,10 +110,6 @@ struct rdma_restrack_entry {
* @type: various objects in restrack database
*/
enum rdma_restrack_type type;
- /**
- * @user: user resource
- */
- bool user;
/**
* @id: ID to expose to users
*/
--
2.34.1On Tue, Feb 14, 2023 at 01:53:52PM +0100, Christophe JAILLET wrote:
> diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> index 8b7c46daeb07..da53fefe6f9e 100644
> --- a/include/rdma/restrack.h
> +++ b/include/rdma/restrack.h
> @@ -80,6 +80,10 @@ struct rdma_restrack_entry {
> * query stage.
> */
> u8 no_track : 1;
> + /**
> + * @user: user resource
> + */
> + bool user;
Can we combine this into the bitfield above?
Jason
Le 14/02/2023 à 14:08, Jason Gunthorpe a écrit :
> On Tue, Feb 14, 2023 at 01:53:52PM +0100, Christophe JAILLET wrote:
>> diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
>> index 8b7c46daeb07..da53fefe6f9e 100644
>> --- a/include/rdma/restrack.h
>> +++ b/include/rdma/restrack.h
>> @@ -80,6 +80,10 @@ struct rdma_restrack_entry {
>> * query stage.
>> */
>> u8 no_track : 1;
>> + /**
>> + * @user: user resource
>> + */
>> + bool user;
>
> Can we combine this into the bitfield above?
>
> Jason
>
Hi,
and even above, we have
bool valid;
I wanted to keep the changes as minimal as possible, but I can change
them all in a single bitfield.
Do you want code such as:
static void rdma_restrack_attach_task(struct rdma_restrack_entry *res,
struct task_struct *task)
{
if (WARN_ON_ONCE(!task))
return;
if (res->task)
put_task_struct(res->task);
get_task_struct(task);
res->task = task;
res->user = true; <--------
}
to be changed with 0/1 instead of false/true?
Apparently gcc 11.3 is fine with using true with u8:1, but I don't find
it really logical.
CJ
On Tue, Feb 14, 2023 at 03:34:21PM +0100, Christophe JAILLET wrote:
> Le 14/02/2023 à 14:08, Jason Gunthorpe a écrit :
> > On Tue, Feb 14, 2023 at 01:53:52PM +0100, Christophe JAILLET wrote:
> > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > > index 8b7c46daeb07..da53fefe6f9e 100644
> > > --- a/include/rdma/restrack.h
> > > +++ b/include/rdma/restrack.h
> > > @@ -80,6 +80,10 @@ struct rdma_restrack_entry {
> > > * query stage.
> > > */
> > > u8 no_track : 1;
> > > + /**
> > > + * @user: user resource
> > > + */
> > > + bool user;
> >
> > Can we combine this into the bitfield above?
> >
> > Jason
> >
> Hi,
>
> and even above, we have
> bool valid;
>
> I wanted to keep the changes as minimal as possible, but I can change them
> all in a single bitfield.
IIRC it needs to be checked, I vaugely remember valid can't be a
bitfield because it is an atomic
> Do you want code such as:
> static void rdma_restrack_attach_task(struct rdma_restrack_entry *res,
> struct task_struct *task)
> {
> if (WARN_ON_ONCE(!task))
> return;
>
> if (res->task)
> put_task_struct(res->task);
> get_task_struct(task);
> res->task = task;
> res->user = true; <--------
> }
>
> to be changed with 0/1 instead of false/true?
I'd keep with true/false
Ideally the bitfield itself would be bool type too
> Apparently gcc 11.3 is fine with using true with u8:1, but I don't find it
> really logical.
Bool types can be casted to integers in defined ways, it is pretty
normal.
Jason
On Tue, Feb 14, 2023 at 11:01:00AM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 14, 2023 at 03:34:21PM +0100, Christophe JAILLET wrote:
> > Le 14/02/2023 à 14:08, Jason Gunthorpe a écrit :
> > > On Tue, Feb 14, 2023 at 01:53:52PM +0100, Christophe JAILLET wrote:
> > > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > > > index 8b7c46daeb07..da53fefe6f9e 100644
> > > > --- a/include/rdma/restrack.h
> > > > +++ b/include/rdma/restrack.h
> > > > @@ -80,6 +80,10 @@ struct rdma_restrack_entry {
> > > > * query stage.
> > > > */
> > > > u8 no_track : 1;
> > > > + /**
> > > > + * @user: user resource
> > > > + */
> > > > + bool user;
> > >
> > > Can we combine this into the bitfield above?
> > >
> > > Jason
> > >
> > Hi,
> >
> > and even above, we have
> > bool valid;
> >
> > I wanted to keep the changes as minimal as possible, but I can change them
> > all in a single bitfield.
>
> IIRC it needs to be checked, I vaugely remember valid can't be a
> bitfield because it is an atomic
I don't remember anything like this.
Thanks
Le 15/02/2023 à 12:31, Leon Romanovsky a écrit :
> On Tue, Feb 14, 2023 at 11:01:00AM -0400, Jason Gunthorpe wrote:
>> On Tue, Feb 14, 2023 at 03:34:21PM +0100, Christophe JAILLET wrote:
>>> Le 14/02/2023 à 14:08, Jason Gunthorpe a écrit :
>>>> On Tue, Feb 14, 2023 at 01:53:52PM +0100, Christophe JAILLET wrote:
>>>>> diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
>>>>> index 8b7c46daeb07..da53fefe6f9e 100644
>>>>> --- a/include/rdma/restrack.h
>>>>> +++ b/include/rdma/restrack.h
>>>>> @@ -80,6 +80,10 @@ struct rdma_restrack_entry {
>>>>> * query stage.
>>>>> */
>>>>> u8 no_track : 1;
>>>>> + /**
>>>>> + * @user: user resource
>>>>> + */
>>>>> + bool user;
>>>>
>>>> Can we combine this into the bitfield above?
>>>>
>>>> Jason
>>>>
>>> Hi,
>>>
>>> and even above, we have
>>> bool valid;
>>>
>>> I wanted to keep the changes as minimal as possible, but I can change them
>>> all in a single bitfield.
>>
>> IIRC it needs to be checked, I vaugely remember valid can't be a
>> bitfield because it is an atomic
>
> I don't remember anything like this.
>
> Thanks
>
If I understand code correctly, 'valid' is only used in
rdma_restrack_add() and rdma_restrack_del().
I don't think that any atomic behavior is in place in these functions.
I'll send in the coming days a v2 which changes 'valid', 'no_track' and
'user' as bool:1.
CJ
© 2016 - 2025 Red Hat, Inc.