On 22.07.22 12:54, Vincent Mailhol wrote:
On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
On 22.07.2022 17:33:08, Vincent Mailhol wrote:
On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
On 22.07.2022 12:20:43, Vincent Mailhol wrote:
I do not buy your argument that you can not do sizeof(struct
canxl_frame) because of naming.
With a flexible array member, I can do:
struct canxl_frame {
canid_t prio; /* 11 bit priority for arbitration (canid_t) */
__u8 flags; /* additional flags for CAN XL */
__u8 sdt; /* SDU (service data unit) type */
__u16 len; /* frame payload length in byte */
__u32 af; /* acceptance field */
__u8 data[];
};
void foo (int s)
{
struct canxl_frame cxl_hdr = { 0 };
struct canxl_frame *cxl1, *cxl2;
size_t cxl2_data_len, cxl2_frame_len;
/* read header */
assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
/* read remaining data */
assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
For performance reasons you probable don't want to split the read of a
single CAN frame over 2 read() syscalls.
Yes, two read() syscalls (with testing/asserting for the right size),
one malloc() syscall, one memcpy().
Alternatively I can offer ONE syscall with ONE single copy operation
from kernel to userspace \o/
read(s, &can.xl, sizeof(struct canxl_frame));
The more I read about splitting/peeking/testing, calculating of sizes,
etc the more I like the simple struct canxl_frame with 2048 bytes of
data that is specified to be truncated by definition.
E.g. for candump or other usual CAN applications you can run with *one*
static struct without any malloc(). This is not only a bridge for
application programmers that have experiences in programming SocketCAN
applications - it is just a safe and simple pattern that I would not
like to give up for which improvement?
In fact I don't know any SocketCAN application that uses malloc for CAN
frames which likely introduces lags and affects the performance.
Best regards,
Oliver
ACK. I wrote the code to illustrate how to do header manipulations.
The goal of this example was not to be optimal but to show off how
sizeof(struct canxl_frame) could be used. Sorry if the example was not
natural and a bit too forced.
Also the CAN_RAW doesn't
support "split"-read now, but could be extended.
Interesting! I naively thought that split read() was handled at a
higher level of the socket API. I did not know that it was per
protocol.
The CAN_RAW protocol implements raw_recvmsg():
| https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L843
It could make sense to allow such split read() for CANXL.
Then we have to track the number of already read bytes inside the
socket. The POSIX API offers some interesting flags to recvmsg(). With
MSG_PEEK you can read but not remove the read data from the queue and/or
MSG_TRUNC to get the total size of the CAN frame.
I have not tested these flags, but I think support for them has to be
implemented in CAN_RAW, too.
Also, we should have a look at the UDP code.
Here it is:
https://elixir.bootlin.com/linux/latest/source/net/ipv4/udp.c#L1846
The relevant function is sk_peek_offset:
https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L617
And to finish, there is an nearly eponym field in struct sock: sk_peek_off
https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L457
One example
I can directly think of is the Packet API. Correct me if I am wrong
but if writing generic code to dump packets on any interfaces, you do
not know in advance the MTU. And so, I guess you have to provide a
buffer of an arbitrary size. A generic program using a buffer of, let
say, 2048 bytes (one page) will not be enough to get a CANXL frame in
one single shot.
Nitpick: the page size is arch and/or kernel config dependent and the
smallest page size that Linux supports is 4k.
Arg, I did not have enough sleep.
By the way, do you have a preference between the flexible array member
and the data[CANXL_MAX_DLEN]? So far, I have been pushing the idea of
the flexible array member but if I am the only one to support this
idea, we can call it a day and use the data[CANXL_MAX_DLEN] approach.
Yours sincerely,
Vincent Mailhol
