Re: [PATCH 1/2] device.h: pack struct dev_links_info
On 2/28/2019 9:35 AM, Greg Kroah-Hartman wrote: On Wed, Feb 27, 2019 at 02:32:26PM +0100, Johan Hovold wrote: On Wed, Feb 27, 2019 at 01:06:45PM +0100, Greg Kroah-Hartman wrote: On Wed, Feb 27, 2019 at 11:59:51AM +0100, Johan Hovold wrote: Yeah, that is a good point, normally we use packed to keep padding from the middle of the structure from happening. I just don't like that 4 bytes sitting there doing nothing :) You could perhaps put them directly in struct device if the dev_links_info struct is just used a separator there and this really bothers you. :) True :) But, in thinking about this, there is no real reason that I can see that this structure even is in struct device. It should be able to be in the private "internal" structure. The patch below moves it out of struct device entirely. Overall there is no memory savings, but it could give us the chance to only create this structure if we really need it later on, as very few things use links at this point in time. Rafael, there is one logic change below, the link structure is not initialized until device_add() happens, instead of device_initialize(). Will that affect anything that you can think of? Does anyone do anything with links before device_add() is called? I think device_add() may be too late. The earliest point in time when device links can be added is after :c:func:`device_add()` has been called for the supplier and :c:func:`device_initialize()` has been called for the consumer. That is true today due to the way the code is set up, but it would be good to figure out if anyone actually does call it this early. ISTR a use case where it was needed in the IOMMU subsystem, but I'm not sure if it has been used that way eventually. The only way to really find out would be to audit all of the device_link_add() callers I'm afraid. Cheers, Rafael
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 02:32:26PM +0100, Johan Hovold wrote: > On Wed, Feb 27, 2019 at 01:06:45PM +0100, Greg Kroah-Hartman wrote: > > On Wed, Feb 27, 2019 at 11:59:51AM +0100, Johan Hovold wrote: > > > Yeah, that is a good point, normally we use packed to keep padding from > > the middle of the structure from happening. > > > > I just don't like that 4 bytes sitting there doing nothing :) > > You could perhaps put them directly in struct device if the > dev_links_info struct is just used a separator there and this really > bothers you. :) True :) > > But, in thinking about this, there is no real reason that I can see that > > this structure even is in struct device. It should be able to be in the > > private "internal" structure. > > > > The patch below moves it out of struct device entirely. Overall there > > is no memory savings, but it could give us the chance to only create > > this structure if we really need it later on, as very few things use > > links at this point in time. > > > > Rafael, there is one logic change below, the link structure is not > > initialized until device_add() happens, instead of device_initialize(). > > Will that affect anything that you can think of? Does anyone do > > anything with links before device_add() is called? > > I think device_add() may be too late. > > The earliest point in time when device links can be added is > after :c:func:`device_add()` has been called for the supplier > and :c:func:`device_initialize()` has been called for the > consumer. That is true today due to the way the code is set up, but it would be good to figure out if anyone actually does call it this early. thanks, greg k-h
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 01:06:45PM +0100, Greg Kroah-Hartman wrote: > On Wed, Feb 27, 2019 at 11:59:51AM +0100, Johan Hovold wrote: > Yeah, that is a good point, normally we use packed to keep padding from > the middle of the structure from happening. > > I just don't like that 4 bytes sitting there doing nothing :) You could perhaps put them directly in struct device if the dev_links_info struct is just used a separator there and this really bothers you. :) > But, in thinking about this, there is no real reason that I can see that > this structure even is in struct device. It should be able to be in the > private "internal" structure. > > The patch below moves it out of struct device entirely. Overall there > is no memory savings, but it could give us the chance to only create > this structure if we really need it later on, as very few things use > links at this point in time. > > Rafael, there is one logic change below, the link structure is not > initialized until device_add() happens, instead of device_initialize(). > Will that affect anything that you can think of? Does anyone do > anything with links before device_add() is called? I think device_add() may be too late. The earliest point in time when device links can be added is after :c:func:`device_add()` has been called for the supplier and :c:func:`device_initialize()` has been called for the consumer. Johan
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 11:59:51AM +0100, Johan Hovold wrote:
> On Wed, Feb 27, 2019 at 10:54:24AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 27, 2019 at 10:40:21AM +0100, Johan Hovold wrote:
> > > On Wed, Feb 27, 2019 at 10:31:04AM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> > > > > On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > > > > > The dev_links_info structure has 4 bytes of padding at the end of it
> > > > > > when embedded in struct device (which is the only place it lives).
> > > > > > To
> > > > > > help reduce the size of struct device pack this structure so we can
> > > > > > take
> > > > > > advantage of the hole with later structure reorganizations.
> > > > > >
> > > > > > Cc: "Rafael J. Wysocki"
> > > > > > Signed-off-by: Greg Kroah-Hartman
> > > > > > ---
> > > > > > include/linux/device.h | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > > > index 6cb4640b6160..b63165276a09 100644
> > > > > > --- a/include/linux/device.h
> > > > > > +++ b/include/linux/device.h
> > > > > > @@ -884,7 +884,7 @@ struct dev_links_info {
> > > > > > struct list_head suppliers;
> > > > > > struct list_head consumers;
> > > > > > enum dl_dev_state status;
> > > > > > -};
> > > > > > +} __packed;
> > > > >
> > > > > This seems like a bad idea. You're changing the alignment of these
> > > > > fields to one byte, something which may cause the compiler to generate
> > > > > less efficient code to deal with unaligned accesses (even if they
> > > > > happen
> > > > > to currently be naturally aligned in struct device).
> > > >
> > > > No, all this changes is the trailing "space" is gone. The alignment of
> > > > the fields did not change at all as they are all naturally aligned
> > > > (list_head is just 2 pointers).
> > >
> > > Yes, currently and in struct device, but given a pointer to a struct
> > > dev_links_info the compiler must assume it is unaligned and act
> > > accordingly for example.
> >
> > Packing the structure doesn't mean that the addressing of it is not also
> > aligned, that should just depend on the location of the pointer in the
> > first place, right?
>
> Packing a structure per definition means changing the alignment
> requirement of each field of the struct to 1-byte alignment.
>
> Another example of unintended consequences would obviously be that if
> someone later adds a short field, say 1-byte, field before the
> dev_links_info struct, all its fields would be non-naturally aligned
> also in struct device.
>
> Sure that can be avoided by inspection (and refusal to add new holes),
> but again, not obvious when the link structure is defined elsewhere.
>
> > Surely compilers are not that foolish :)
> >
> > And accessing this field should not be an issue of "slow", hopefully the
> > memory savings would offset any compiler mess.
>
> There are other subtleties like atomicity that may come into play.
>
> And even if any penalties are deemed acceptable in this case, you're
> also setting a precedent for others. Note that we do not seem to use
> __packed this way currently
Yeah, that is a good point, normally we use packed to keep padding from
the middle of the structure from happening.
I just don't like that 4 bytes sitting there doing nothing :)
> > > > So this allows us to save 4 bytes in struct device by putting something
> > > > in that
> > > > trailing "hole" that can be aligned with it better (i.e. an integer or
> > > > something else).
> > >
> > > I understand that, but I don't think it is worth to start using packed
> > > liked this for internal structures as it may have subtle and unintended
> > > consequences.
> >
> > I'm not understanding what the consequences are here, sorry. Does the
> > compiler output change given that the structure is still aligned
> > properly in the "parent" structure? I can't see any output changed
> > here, but maybe I am not looking properly?
>
> It's all arch dependent, and you won't see any difference on x86-64.
>
> The following example produces additional instructions even on 32-bit
> arm here:
>
> struct a1 {
> void *p;
> void *q;
> int i;
> } __attribute__((__packed__));
>
> struct a2 {
> void *p;
> void *q;
> int i;
> };
>
> int f(struct a1 *a)
> {
> return a->i;
> }
>
> int g(struct a2 *a)
> {
> return a->i;
> }
Ok, fair enough, I'll leave this alone.
But, in thinking about this, there is no real reason that I can see that
this structure even is in struct device. It should be able to be in the
private "internal" structure.
The patch below moves it out of struct device entirely. Overall there
is no memory savings, but it could give us the chance to only create
this structure if we really need it later on, as very few things use
links at this point in time.
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 10:54:24AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 27, 2019 at 10:40:21AM +0100, Johan Hovold wrote:
> > On Wed, Feb 27, 2019 at 10:31:04AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> > > > On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > > > > The dev_links_info structure has 4 bytes of padding at the end of it
> > > > > when embedded in struct device (which is the only place it lives). To
> > > > > help reduce the size of struct device pack this structure so we can
> > > > > take
> > > > > advantage of the hole with later structure reorganizations.
> > > > >
> > > > > Cc: "Rafael J. Wysocki"
> > > > > Signed-off-by: Greg Kroah-Hartman
> > > > > ---
> > > > > include/linux/device.h | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > > index 6cb4640b6160..b63165276a09 100644
> > > > > --- a/include/linux/device.h
> > > > > +++ b/include/linux/device.h
> > > > > @@ -884,7 +884,7 @@ struct dev_links_info {
> > > > > struct list_head suppliers;
> > > > > struct list_head consumers;
> > > > > enum dl_dev_state status;
> > > > > -};
> > > > > +} __packed;
> > > >
> > > > This seems like a bad idea. You're changing the alignment of these
> > > > fields to one byte, something which may cause the compiler to generate
> > > > less efficient code to deal with unaligned accesses (even if they happen
> > > > to currently be naturally aligned in struct device).
> > >
> > > No, all this changes is the trailing "space" is gone. The alignment of
> > > the fields did not change at all as they are all naturally aligned
> > > (list_head is just 2 pointers).
> >
> > Yes, currently and in struct device, but given a pointer to a struct
> > dev_links_info the compiler must assume it is unaligned and act
> > accordingly for example.
>
> Packing the structure doesn't mean that the addressing of it is not also
> aligned, that should just depend on the location of the pointer in the
> first place, right?
Packing a structure per definition means changing the alignment
requirement of each field of the struct to 1-byte alignment.
Another example of unintended consequences would obviously be that if
someone later adds a short field, say 1-byte, field before the
dev_links_info struct, all its fields would be non-naturally aligned
also in struct device.
Sure that can be avoided by inspection (and refusal to add new holes),
but again, not obvious when the link structure is defined elsewhere.
> Surely compilers are not that foolish :)
>
> And accessing this field should not be an issue of "slow", hopefully the
> memory savings would offset any compiler mess.
There are other subtleties like atomicity that may come into play.
And even if any penalties are deemed acceptable in this case, you're
also setting a precedent for others. Note that we do not seem to use
__packed this way currently
> > > So this allows us to save 4 bytes in struct device by putting something
> > > in that
> > > trailing "hole" that can be aligned with it better (i.e. an integer or
> > > something else).
> >
> > I understand that, but I don't think it is worth to start using packed
> > liked this for internal structures as it may have subtle and unintended
> > consequences.
>
> I'm not understanding what the consequences are here, sorry. Does the
> compiler output change given that the structure is still aligned
> properly in the "parent" structure? I can't see any output changed
> here, but maybe I am not looking properly?
It's all arch dependent, and you won't see any difference on x86-64.
The following example produces additional instructions even on 32-bit
arm here:
struct a1 {
void *p;
void *q;
int i;
} __attribute__((__packed__));
struct a2 {
void *p;
void *q;
int i;
};
int f(struct a1 *a)
{
return a->i;
}
int g(struct a2 *a)
{
return a->i;
}
Johan
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 10:40:21AM +0100, Johan Hovold wrote:
> On Wed, Feb 27, 2019 at 10:31:04AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> > > On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > > > The dev_links_info structure has 4 bytes of padding at the end of it
> > > > when embedded in struct device (which is the only place it lives). To
> > > > help reduce the size of struct device pack this structure so we can take
> > > > advantage of the hole with later structure reorganizations.
> > > >
> > > > Cc: "Rafael J. Wysocki"
> > > > Signed-off-by: Greg Kroah-Hartman
> > > > ---
> > > > include/linux/device.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index 6cb4640b6160..b63165276a09 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -884,7 +884,7 @@ struct dev_links_info {
> > > > struct list_head suppliers;
> > > > struct list_head consumers;
> > > > enum dl_dev_state status;
> > > > -};
> > > > +} __packed;
> > >
> > > This seems like a bad idea. You're changing the alignment of these
> > > fields to one byte, something which may cause the compiler to generate
> > > less efficient code to deal with unaligned accesses (even if they happen
> > > to currently be naturally aligned in struct device).
> >
> > No, all this changes is the trailing "space" is gone. The alignment of
> > the fields did not change at all as they are all naturally aligned
> > (list_head is just 2 pointers).
>
> Yes, currently and in struct device, but given a pointer to a struct
> dev_links_info the compiler must assume it is unaligned and act
> accordingly for example.
Packing the structure doesn't mean that the addressing of it is not also
aligned, that should just depend on the location of the pointer in the
first place, right?
Surely compilers are not that foolish :)
And accessing this field should not be an issue of "slow", hopefully the
memory savings would offset any compiler mess.
> > So this allows us to save 4 bytes in struct device by putting something in
> > that
> > trailing "hole" that can be aligned with it better (i.e. an integer or
> > something else).
>
> I understand that, but I don't think it is worth to start using packed
> liked this for internal structures as it may have subtle and unintended
> consequences.
I'm not understanding what the consequences are here, sorry. Does the
compiler output change given that the structure is still aligned
properly in the "parent" structure? I can't see any output changed
here, but maybe I am not looking properly?
thanks,
greg k-h
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 10:31:04AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> > On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > > The dev_links_info structure has 4 bytes of padding at the end of it
> > > when embedded in struct device (which is the only place it lives). To
> > > help reduce the size of struct device pack this structure so we can take
> > > advantage of the hole with later structure reorganizations.
> > >
> > > Cc: "Rafael J. Wysocki"
> > > Signed-off-by: Greg Kroah-Hartman
> > > ---
> > > include/linux/device.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 6cb4640b6160..b63165276a09 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -884,7 +884,7 @@ struct dev_links_info {
> > > struct list_head suppliers;
> > > struct list_head consumers;
> > > enum dl_dev_state status;
> > > -};
> > > +} __packed;
> >
> > This seems like a bad idea. You're changing the alignment of these
> > fields to one byte, something which may cause the compiler to generate
> > less efficient code to deal with unaligned accesses (even if they happen
> > to currently be naturally aligned in struct device).
>
> No, all this changes is the trailing "space" is gone. The alignment of
> the fields did not change at all as they are all naturally aligned
> (list_head is just 2 pointers).
Yes, currently and in struct device, but given a pointer to a struct
dev_links_info the compiler must assume it is unaligned and act
accordingly for example.
> So this allows us to save 4 bytes in struct device by putting something in
> that
> trailing "hole" that can be aligned with it better (i.e. an integer or
> something else).
I understand that, but I don't think it is worth to start using packed
liked this for internal structures as it may have subtle and unintended
consequences.
Johan
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Wed, Feb 27, 2019 at 10:23:18AM +0100, Johan Hovold wrote:
> On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> > The dev_links_info structure has 4 bytes of padding at the end of it
> > when embedded in struct device (which is the only place it lives). To
> > help reduce the size of struct device pack this structure so we can take
> > advantage of the hole with later structure reorganizations.
> >
> > Cc: "Rafael J. Wysocki"
> > Signed-off-by: Greg Kroah-Hartman
> > ---
> > include/linux/device.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 6cb4640b6160..b63165276a09 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -884,7 +884,7 @@ struct dev_links_info {
> > struct list_head suppliers;
> > struct list_head consumers;
> > enum dl_dev_state status;
> > -};
> > +} __packed;
>
> This seems like a bad idea. You're changing the alignment of these
> fields to one byte, something which may cause the compiler to generate
> less efficient code to deal with unaligned accesses (even if they happen
> to currently be naturally aligned in struct device).
No, all this changes is the trailing "space" is gone. The alignment of
the fields did not change at all as they are all naturally aligned
(list_head is just 2 pointers).
Here's the pahole output before and after this patch:
Before:
struct dev_links_info {
struct list_head suppliers;/* 016 */
struct list_head consumers;/*1616 */
enum dl_dev_state status; /*32 4 */
/* size: 40, cachelines: 1, members: 3 */
/* padding: 4 */
/* last cacheline: 40 bytes */
};
After:
struct dev_links_info {
struct list_head suppliers;/* 016 */
struct list_head consumers;/*1616 */
enum dl_dev_state status; /*32 4 */
/* size: 36, cachelines: 1, members: 3 */
/* last cacheline: 36 bytes */
};
So this allows us to save 4 bytes in struct device by putting something in that
trailing "hole" that can be aligned with it better (i.e. an integer or
something else).
thanks,
greg k-h
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On Tue, Feb 26, 2019 at 03:41:07PM +0100, Greg Kroah-Hartman wrote:
> The dev_links_info structure has 4 bytes of padding at the end of it
> when embedded in struct device (which is the only place it lives). To
> help reduce the size of struct device pack this structure so we can take
> advantage of the hole with later structure reorganizations.
>
> Cc: "Rafael J. Wysocki"
> Signed-off-by: Greg Kroah-Hartman
> ---
> include/linux/device.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 6cb4640b6160..b63165276a09 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -884,7 +884,7 @@ struct dev_links_info {
> struct list_head suppliers;
> struct list_head consumers;
> enum dl_dev_state status;
> -};
> +} __packed;
This seems like a bad idea. You're changing the alignment of these
fields to one byte, something which may cause the compiler to generate
less efficient code to deal with unaligned accesses (even if they happen
to currently be naturally aligned in struct device).
I don't think we should mess with __packed unless for things that
actually require it (e.g. data going on to the wire) even if it means
wasting 4 bytes on 64-bit archs.
Johan
Re: [PATCH 1/2] device.h: pack struct dev_links_info
On 2/26/2019 3:41 PM, Greg Kroah-Hartman wrote:
The dev_links_info structure has 4 bytes of padding at the end of it
when embedded in struct device (which is the only place it lives). To
help reduce the size of struct device pack this structure so we can take
advantage of the hole with later structure reorganizations.
Cc: "Rafael J. Wysocki"
Signed-off-by: Greg Kroah-Hartman
Reviewed-by: Rafael J. Wysocki
---
include/linux/device.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/device.h b/include/linux/device.h
index 6cb4640b6160..b63165276a09 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -884,7 +884,7 @@ struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
enum dl_dev_state status;
-};
+} __packed;
/**
* struct device - The basic device structure
