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? > nbytes = read(s, &can.xl, sizeof(struct canxl_frame)); > if (nbytes < 0) { > perror("read"); > return 1; > } > printf("nbytes = %d\n", nbytes); Does read() guarantee atomicity? From "man 2 read": | It is not an error if [the return value] is smaller than the number | of bytes requested; this may happen for example because fewer bytes | are actually available right now (maybe because we were close to | end-of-file, or because we are reading from a pipe, or from a | terminal), *or because read() was interrupted by a signal*. I think the answer is yes, but I prefer to double check (I am especially concerned by the signal interrupts). > > if (nbytes < CANXL_HDR_SZ + CANXL_MIN_DLEN) { > fprintf(stderr, "read: no CAN frame\n"); > return 1; > } > > if (can.xl.flags & CANXL_XLF) { > if (nbytes != CANXL_HDR_SZ + can.xl.len) { > printf("nbytes = %d\n", nbytes); > perror("read canxl_frame"); ACK. > continue; > } > /* process CAN XL frames */ > printf("canxl frame prio %03X len %d flags %d\n", > can.xl.prio, can.xl.len, can.xl.flags); > continue; > } > > if (nbytes != sizeof(struct can_frame) && > nbytes != sizeof(struct canfd_frame)) { On the first read, I thought you meant "else if", then, I saw the "continue" on the previous line. > fprintf(stderr, "read: incomplete CAN(FD) frame\n"); > return 1; > } > > /* process CAN/FD frames */ > > Working with partially filled data structures is a known pattern for CAN > and CAN FD. Except that for CAN-(FD), truncation is not possible. > We only optimize the transfer from/to kernel space with truncated > read/write operations. > > ¯\_(ツ)_/¯ Yes, this full discussion is about optimizations… > > > > Maybe even better: > > > > switch(ntohs(skb->protocol)) { > > case ETH_P_CAN: > > /* ... */ > > case ETH_P_CANFD: > > /* ... */ > > case ETH_P_CANXL: > > /* ... */ > > default: > > /* ... */ > > } > > Yes ... updated my next patch. > > Best regards, > Oliver
