[PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Ricardo Ribalda]

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(-)

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;

---
base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f
change-id: 20230501-uvc-align-6ff202b68dab

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda