On 21.07.22 04:37, Vincent Mailhol 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.
Yes!
Btw. How should we finally name the 'non data' header of CAN XL?
1. CANXL_HEADER_SIZE
2. CANXL_HEAD_SIZE
3. CANXL_HDR_SIZE
4. CANXL_HDR_SZ <- currently in the patches
5. CANXL_HD_SZ
I think it has to be 'head' and not 'header'.
In skbs we also have head and tail.
So I would vote for 2 or 5 with a tendency to 5.
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.
ACK
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?
Yes, that was just a userland example to read and write some CAN XL
frames along with CAN/CANFD frames with the 'new' CAN_RAW socket.
I just wanted to get an impression whether it is handy to program this
extended API or not.
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).
Hm, we are not reading from a file but from a socket here that provide
chunks in the form of struct can_frame in raw_recvmsg(). You only get a
MSG_TRUNC there when you provide a (buffer)size in userspace that's to
small.
I've never got any error reports on CAN_RAW reading (over 16 years) and
all the examples contain a test for sizeof(struct can_frame) like this:
>> if (nbytes != sizeof(struct can_frame) &&
>> nbytes != sizeof(struct canfd_frame)) {
So we either have an error or an incomplete CAN frame which becomes an
error too.
Do you think this is still worth an investigation?
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.
When we checked for a complete header this really seems to be simple.
And we directly prove CANXL_MIN_DLEN and CANXL_MAX_DLEN is ensured by
read().
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.
Yes. I just wanted to look if I got a CAN XL frame and print its attributes.
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.
Exactly. CAN/FD uses the fixed structure sizes to distinguish the frame
types.
We only optimize the transfer from/to kernel space with truncated
read/write operations.
¯\_(ツ)_/¯
Yes, this full discussion is about optimizations…
Optimization and STYLE ;-)
Thanks for your feedback!
Best regards,
Oliver
