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