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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > > 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