| Message ID | 20230501-uvc-align-v1-1-0f713e4b84c3@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Explicit alignment of uvc_frame and uvc_format | expand |
Hi Ricardo, Thank you for the patch. On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote: > Struct uvc_frame and uvc_format are packaged together on > streaming->formats on a sigle allocation. s/sigle/single/ > > This is working fine because both structures have a field with a > pointer, but it will stop working when the sizeof() of any of those > structs is not a muliple of the sizeof(void*). > > Make that aligment contract explicit. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > This is better than 3 allocations, and do not have any performance > penalty. > --- > drivers/media/usb/uvc/uvcvideo.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 9a596c8d894a..03e8a543c8e6 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -252,7 +252,7 @@ struct uvc_frame { > u8 bFrameIntervalType; > u32 dwDefaultFrameInterval; > u32 *dwFrameInterval; > -}; > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */ Don't we need u32 alignment here, not void * alignment, given that uvc_frame is followed by an array of u32 ? > > struct uvc_format { > u8 type; > @@ -266,7 +266,7 @@ struct uvc_format { > > unsigned int nframes; > struct uvc_frame *frame; > -}; > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */ Same here, technically we need to ensure that the following uvc_frame will be aligned. void * alignment will give us that now, but that's not the actual constraint. Wouldn't it be better to handle the alignment constraints explicitly when allocating the memory ? It's not that uvc_frame and uvc_format have intrinsic alignment constraints, the constraints are only needed because of the way memory is allocated. > > struct uvc_streaming_header { > u8 bNumFormats; > > --- > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f > change-id: 20230501-uvc-align-6ff202b68dab
Hi Laurent On Fri, 22 Mar 2024 at 12:56, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote: > > Struct uvc_frame and uvc_format are packaged together on > > streaming->formats on a sigle allocation. > > s/sigle/single/ > > > > > This is working fine because both structures have a field with a > > pointer, but it will stop working when the sizeof() of any of those > > structs is not a muliple of the sizeof(void*). > > > > Make that aligment contract explicit. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > This is better than 3 allocations, and do not have any performance > > penalty. > > --- > > drivers/media/usb/uvc/uvcvideo.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 9a596c8d894a..03e8a543c8e6 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -252,7 +252,7 @@ struct uvc_frame { > > u8 bFrameIntervalType; > > u32 dwDefaultFrameInterval; > > u32 *dwFrameInterval; > > -}; > > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */ > > Don't we need u32 alignment here, not void * alignment, given that > uvc_frame is followed by an array of u32 ? Let me make sure that I explain myself :) I made a small program in compiler explorer: https://godbolt.org/z/7s9z8WTsx that shows the error that I want to avoid When we have a structure like this: struct n_foo_bar { int n; struct foo *foo; struct bar *bar; }; We expect that *foo and *bar point to memory addresses with the right cpu alignment for each struct. Otherwise accessing foo and bar could be slow or simply not work. In the driver we are doing something like this to allocate the structure: int size struct n_foo_bar *out; size = n*sizeof(struct foo)+n*sizeof(struct bar) +sizeof(struct n_foo_bar); out = malloc(size); if (!out) return out; out->foo=(void *)(out)+sizeof(struct n_foo_bar); out->bar=(void *)(out->foo)+n*sizeof(struct foo); But that only works if sizeof(struct foo) is a multiple of the alignment required by struct bar. We are "lucky" now because we have a pointer in each struct and that gives us a void* padding. ... but if we ever remove that pointer from the structure we will be in a bad position. With the __aligned(sizeof(void *)); I want to explicitly say: "Ey, this struct is embedded in another struct and they are allocated contiguously" Does it make more sense now? > > > > > struct uvc_format { > > u8 type; > > @@ -266,7 +266,7 @@ struct uvc_format { > > > > unsigned int nframes; > > struct uvc_frame *frame; > > -}; > > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */ > > Same here, technically we need to ensure that the following uvc_frame > will be aligned. void * alignment will give us that now, but that's not > the actual constraint. > > Wouldn't it be better to handle the alignment constraints explicitly > when allocating the memory ? It's not that uvc_frame and uvc_format have > intrinsic alignment constraints, the constraints are only needed because > of the way memory is allocated. > > > > > struct uvc_streaming_header { > > u8 bNumFormats; > > > > --- > > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f > > change-id: 20230501-uvc-align-6ff202b68dab > > -- > Regards, > > Laurent Pinchart
Hi Ricardo, On Fri, Mar 22, 2024 at 03:26:39PM +0100, Ricardo Ribalda wrote: > On Fri, 22 Mar 2024 at 12:56, Laurent Pinchart wrote: > > On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote: > > > Struct uvc_frame and uvc_format are packaged together on > > > streaming->formats on a sigle allocation. > > > > s/sigle/single/ > > > > > This is working fine because both structures have a field with a > > > pointer, but it will stop working when the sizeof() of any of those > > > structs is not a muliple of the sizeof(void*). > > > > > > Make that aligment contract explicit. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > This is better than 3 allocations, and do not have any performance > > > penalty. > > > --- > > > drivers/media/usb/uvc/uvcvideo.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 9a596c8d894a..03e8a543c8e6 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -252,7 +252,7 @@ struct uvc_frame { > > > u8 bFrameIntervalType; > > > u32 dwDefaultFrameInterval; > > > u32 *dwFrameInterval; > > > -}; > > > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */ > > > > Don't we need u32 alignment here, not void * alignment, given that > > uvc_frame is followed by an array of u32 ? > > Let me make sure that I explain myself :) > > I made a small program in compiler explorer: > https://godbolt.org/z/7s9z8WTsx that shows the error that I want to > avoid > > When we have a structure like this: > > struct n_foo_bar { > int n; > struct foo *foo; > struct bar *bar; > }; > > We expect that *foo and *bar point to memory addresses with the right > cpu alignment for each struct. Otherwise accessing foo and bar could > be slow or simply not work. So far, so good. > In the driver we are doing something like this to allocate the structure: > > int size > struct n_foo_bar *out; > > size = n*sizeof(struct foo)+n*sizeof(struct bar) +sizeof(struct n_foo_bar); > out = malloc(size); > if (!out) > return out; > > out->foo=(void *)(out)+sizeof(struct n_foo_bar); > out->bar=(void *)(out->foo)+n*sizeof(struct foo); > > But that only works if sizeof(struct foo) is a multiple of the > alignment required by struct bar. The real requirement is a bit more complex, it's sizeof(struct n_foo_bar) + sizeof(struct foo) that needs to be a multiple of the alignment required by struct bar (and even that is simplified, as it assumes that malloc() returns a pointer aligned to the requirements of struct bar, which in practice should always be the case). > We are "lucky" now because we have a > pointer in each struct and that gives us a void* padding. ... but if > we ever remove that pointer from the structure we will be in a bad > position. We have three levels in uvcvideo. The top-level structure (your equivalent of n_foo_bar), struct uvc_format, has a pointer to an array of struct uvc_frame. The second level, struct uvc_frame, has a pointer to an array of u32. All three are then allocated in one go, contiguously. The largest field in uvc_frame is a pointer, so the alignment requirement will be fulfilled if struct uvc_format is aligned to sizeof(void *). When it comes to struct uvc_frame, however, its size needs to be a multiple of sizeof(u32), not of sizeof(void *). Given that the alignment constraints are not intrinsic to these structures, I think it would be better to handle them when allocating the memory. Something along the line of diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index f33a01dbb329..cbc40d663e4f 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -687,8 +687,11 @@ static int uvc_parse_streaming(struct uvc_device *dev, goto error; } - size = nformats * sizeof(*format) + nframes * sizeof(*frame) + size = nformats * sizeof(*format); + size = ALIGN(size, __alignof__(*frame)) + nframes * sizeof(*frame); + size = ALIGN(size, __alignof__(*interval)) + nintervals * sizeof(*interval); + format = kzalloc(size, GFP_KERNEL); if (format == NULL) { ret = -ENOMEM; plus a corresponding change when calculating the pointers to the frames and intervals just after. > With the __aligned(sizeof(void *)); I want to explicitly say: > > "Ey, this struct is embedded in another struct and they are allocated > contiguously" > > Does it make more sense now? > > > > > > > struct uvc_format { > > > u8 type; > > > @@ -266,7 +266,7 @@ struct uvc_format { > > > > > > unsigned int nframes; > > > struct uvc_frame *frame; > > > -}; > > > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */ > > > > Same here, technically we need to ensure that the following uvc_frame > > will be aligned. void * alignment will give us that now, but that's not > > the actual constraint. > > > > Wouldn't it be better to handle the alignment constraints explicitly > > when allocating the memory ? It's not that uvc_frame and uvc_format have > > intrinsic alignment constraints, the constraints are only needed because > > of the way memory is allocated. > > > > > > > > struct uvc_streaming_header { > > > u8 bNumFormats; > > > > > > --- > > > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f > > > change-id: 20230501-uvc-align-6ff202b68dab
Hi Laurent On Thu, 4 Apr 2024 at 03:04, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Fri, Mar 22, 2024 at 03:26:39PM +0100, Ricardo Ribalda wrote: > > On Fri, 22 Mar 2024 at 12:56, Laurent Pinchart wrote: > > > On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote: > > > > Struct uvc_frame and uvc_format are packaged together on > > > > streaming->formats on a sigle allocation. > > > > > > s/sigle/single/ > > > > > > > This is working fine because both structures have a field with a > > > > pointer, but it will stop working when the sizeof() of any of those > > > > structs is not a muliple of the sizeof(void*). > > > > > > > > Make that aligment contract explicit. > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > This is better than 3 allocations, and do not have any performance > > > > penalty. > > > > --- > > > > drivers/media/usb/uvc/uvcvideo.h | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > index 9a596c8d894a..03e8a543c8e6 100644 > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > @@ -252,7 +252,7 @@ struct uvc_frame { > > > > u8 bFrameIntervalType; > > > > u32 dwDefaultFrameInterval; > > > > u32 *dwFrameInterval; > > > > -}; > > > > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */ > > > > > > Don't we need u32 alignment here, not void * alignment, given that > > > uvc_frame is followed by an array of u32 ? > > > > Let me make sure that I explain myself :) > > > > I made a small program in compiler explorer: > > https://godbolt.org/z/7s9z8WTsx that shows the error that I want to > > avoid > > > > When we have a structure like this: > > > > struct n_foo_bar { > > int n; > > struct foo *foo; > > struct bar *bar; > > }; > > > > We expect that *foo and *bar point to memory addresses with the right > > cpu alignment for each struct. Otherwise accessing foo and bar could > > be slow or simply not work. > > So far, so good. > > > In the driver we are doing something like this to allocate the structure: > > > > int size > > struct n_foo_bar *out; > > > > size = n*sizeof(struct foo)+n*sizeof(struct bar) +sizeof(struct n_foo_bar); > > out = malloc(size); > > if (!out) > > return out; > > > > out->foo=(void *)(out)+sizeof(struct n_foo_bar); > > out->bar=(void *)(out->foo)+n*sizeof(struct foo); > > > > But that only works if sizeof(struct foo) is a multiple of the > > alignment required by struct bar. > > The real requirement is a bit more complex, it's sizeof(struct n_foo_bar) + > sizeof(struct foo) that needs to be a multiple of the alignment required > by struct bar (and even that is simplified, as it assumes that malloc() > returns a pointer aligned to the requirements of struct bar, which in > practice should always be the case). > struct n_foo_bar, has two pointers: foo and bar. Because of the padding, Its sizeof has to be a multiple of sizeof(void *). We only care about the sizeof(foo). And malloc has to provide an alignment of at least sizeof(void *), otherwise the implementation is pretty broken :) for kmalloc the alignment is ARCH_KMALLOC_MINALIGN > > We are "lucky" now because we have a > > pointer in each struct and that gives us a void* padding. ... but if > > we ever remove that pointer from the structure we will be in a bad > > position. > > We have three levels in uvcvideo. The top-level structure (your > equivalent of n_foo_bar), struct uvc_format, has a pointer to an array > of struct uvc_frame. The second level, struct uvc_frame, has a pointer > to an array of u32. All three are then allocated in one go, > contiguously. > > The largest field in uvc_frame is a pointer, so the alignment > requirement will be fulfilled if struct uvc_format is aligned to > sizeof(void *). When it comes to struct uvc_frame, however, its size > needs to be a multiple of sizeof(u32), not of sizeof(void *). OK, we might save 2 bytes :), at the cost that we cannot reshuffle the fields in the top-level struct. > > Given that the alignment constraints are not intrinsic to these > structures, I think it would be better to handle them when allocating > the memory. Something along the line of This is what I was trying to avoid, but with the __alignof__ macros it does not look that bad... Maybe we should just make 3 allocations instead of having our mini malloc implementation :) Let me send a v2 Thanks! > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index f33a01dbb329..cbc40d663e4f 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -687,8 +687,11 @@ static int uvc_parse_streaming(struct uvc_device *dev, > goto error; > } > > - size = nformats * sizeof(*format) + nframes * sizeof(*frame) > + size = nformats * sizeof(*format); > + size = ALIGN(size, __alignof__(*frame)) + nframes * sizeof(*frame); > + size = ALIGN(size, __alignof__(*interval)) > + nintervals * sizeof(*interval); > + > format = kzalloc(size, GFP_KERNEL); > if (format == NULL) { > ret = -ENOMEM; > > plus a corresponding change when calculating the pointers to the frames > and intervals just after. > > > With the __aligned(sizeof(void *)); I want to explicitly say: > > > > "Ey, this struct is embedded in another struct and they are allocated > > contiguously" > > > > Does it make more sense now? > > > > > > > > > > struct uvc_format { > > > > u8 type; > > > > @@ -266,7 +266,7 @@ struct uvc_format { > > > > > > > > unsigned int nframes; > > > > struct uvc_frame *frame; > > > > -}; > > > > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */ > > > > > > Same here, technically we need to ensure that the following uvc_frame > > > will be aligned. void * alignment will give us that now, but that's not > > > the actual constraint. > > > > > > Wouldn't it be better to handle the alignment constraints explicitly > > > when allocating the memory ? It's not that uvc_frame and uvc_format have > > > intrinsic alignment constraints, the constraints are only needed because > > > of the way memory is allocated. > > > > > > > > > > > struct uvc_streaming_header { > > > > u8 bNumFormats; > > > > > > > > --- > > > > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f > > > > change-id: 20230501-uvc-align-6ff202b68dab > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 9a596c8d894a..03e8a543c8e6 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -252,7 +252,7 @@ struct uvc_frame { u8 bFrameIntervalType; u32 dwDefaultFrameInterval; u32 *dwFrameInterval; -}; +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */ struct uvc_format { u8 type; @@ -266,7 +266,7 @@ struct uvc_format { unsigned int nframes; struct uvc_frame *frame; -}; +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */ struct uvc_streaming_header { u8 bNumFormats;
Struct uvc_frame and uvc_format are packaged together on streaming->formats on a sigle allocation. This is working fine because both structures have a field with a pointer, but it will stop working when the sizeof() of any of those structs is not a muliple of the sizeof(void*). Make that aligment contract explicit. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- This is better than 3 allocations, and do not have any performance penalty. --- drivers/media/usb/uvc/uvcvideo.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f change-id: 20230501-uvc-align-6ff202b68dab Best regards,