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.
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.
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;
nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
if (nbytes < 0) {
perror("read");
return 1;
}
printf("nbytes = %d\n", nbytes);
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");
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)) {
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.
We only optimize the transfer from/to kernel space with truncated
read/write operations.
¯\_(ツ)_/¯
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
