On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol <vincent.mailhol@xxxxxxxxx> wrote: > On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > > On 19.07.22 17:16, Vincent Mailhol wrote: > > > On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > > >> No confusion. > > >> > > >> The API to the user space is 'truncated' option only. > > >> > > >> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame). > > >> > > >> See: > > >> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@xxxxxxxxxxxx/ > > >> > > >> ---8<--- > > >> > > >> As the sk_buffs are only allocated once and are not copied inside the > > >> kernel there should be no remarkable drawbacks whether we allocate > > >> CAN_MTU skbs or CANXL_MTU skbs. > > >> > > >> AFAICS both skbs will fit into a single page. > > > > > > This is true. A page is at least 4k. So the 2k + alpha will easily fit. > > > But the page is not the smallest chunk that can return malloc, c.f. > > > KMALLOC_MIN_SIZE: > > > https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279 > > > > > > Also, more than the page size, my concern are the cache misses, > > > especially when memsetting to zero the full canxl frame. As far as I > > > understand, cloning an skb does not copy the payload (only increment a > > > reference to it) so the echo_skb thing should not be impacted. > > > That said, I am not able to tell whether or not this will have a > > > noticeable impact (I have some feeling it might but can not assert > > > it). If this looks good for the people who have the expertise in > > > kernel performances, then I am fine. > > > > The more I think about our discussion and your remark that we were > > somehow going back to the V2 patch set the more I wonder if this would > > be a good idea. > > I quite liked v2. My comments on the v2 were mostly to argue on the > data[CANXL_MAX_DLEN] vs the flexible member array, but aside from > that, it looked pretty good. > > > IMO using the struct canxl_frame (including 2048 byte) and allowing > > truncated sizes can be handled inside the kernel safely. > > > > And with V2 we only allocate the needed size for the sk_buff - without > > any memsetting. > > > > When user space gets a truncated frame -> fine > > > > When the user forges some CAN XL frame where the length value does not > > match the spec and the size does not fit the length -> -EINVAL > > > > So there is no uninitialized data also. > > So basically, forcing the truncation everywhere (TX, RX both userland > and kernel), correct? i.e. the skb length shall always be equal to > CANXL_HEADER_SIZE + canxl_frame::len. > > I think this is good. As I stated before, getting -EINVAL is benign. > If developers are doing crazy things because they did not read the > doc, it is better to fail them early. If we go for truncation then > always truncating is the safest: less option -> less confusion. > > > And even the user space side to handle a mix of CAN frame types is > > pretty simple IMO: > > > > union { > > struct can_frame cc; > > struct canfd_frame fd; > > struct canxl_frame xl; > > } can; > > Do you want to add this union in the kernel uapi or is it just a > userland example? More brainstorming. If we want to introduce a generic can structure in linux/can.h, we could do: struct canxl_frame { canid_t prio; /* 11 bit priority for arbitration (canid_t) */ __u8 xl_flags; /* additional flags for CAN XL */ __u8 fd_flags; /* CAN(-FD) flags */ __u16 len; /* frame payload length in byte */ __u32 af; /* acceptance field */ __u8 sdt; /* SDU (service data unit) type */ __u8 __res0; /* reserved / padding */ __u8 __res1; /* reserved / padding */ __u8 __res2; /* reserved / padding */ __u8 data[CANXL_MAX_DLEN] __attribute__((aligned(8))); }; union can_generic_frame { struct { union { canid_t can_id; canid_t prio; }; union { __u16 type; struct { __u8 xl_flags; __u8 fd_flags; } __attribute__((packed)); } __attribute__((packed)); }; struct can_frame cc; struct canfd_frame fd; struct canxl_frame xl; }; #define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */ #define CAN_TYPE_CC 0 #ifdef __LITTLE_ENDIAN #define CAN_TYPE_FD (CANFD_FDF << 8) #define CAN_TYPE_XL (CANXL_XLF) #else /* __BIG_ENDIAN */ #define CAN_TYPE_FD (CANFD_FDF) #define CAN_TYPE_XL (CANXL_XLF << 8) #endif #define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL) Because can_generic_frame::type overlaps with the can(fd)_frame::len, it will contain garbage and thus it is necessary to mask it with CAN_TYPE_MASK. The CANFD_FDF is only set in the rx path. In the tx path it is simply ignored. This done, we can use it as below when *receiving* can frames: int foo() { union can_generic_frame can; /* receive a frame */ switch (can.type & CAN_TYPE_MASK) { case CAN_TYPE_CC: printf("Received classical CAN Frame\n"); break; case CAN_TYPE_FD: printf("Received CAN FD Frame\n"); break; case CAN_TYPE_XL: printf("Received CAN XL Frame\n"); break; default: fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK); } return EXIT_SUCCESS; } Yours sincerely, Vincent Mailhol
