On Sat. 23 Jul. 2022 at 00:30, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> 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));
You just ignored what I wrote just below:
| 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.
I was answering your "This is no sizeof(struct canxl_frame) anymore" comment.
You can do:
| struct canxl_frame *cxl = malloc(CANXL_MTU);
| read(s, cxl, CANXL_MTU);
In fact, the sizeof() will be mostly useful in the tx path when doing
write(), not in the rx when you will likely always provide a buffer of
maximum size. Now I regret sending my example.
> 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?
The possibility to do static declaration is the strongest argument in
your favor.
There is no easy way to do this with flexible array member aside maybe
of the convoluted:
| char buf[CANXL_MTU];
| struct canxl_frame *cxl = &(struct canxl_frame)buf;
But I do not see the problem of using dynamic memory. For a 2
kilobytes buffer, dynamic memory looks pretty standard to me. And one
more time, is there any precedent in the kernel uapi of not using
flexible array members for structures meant to hold more than one
kilobyte? For the tiny CAN(-FD) it made sense, the same argument does
not translate so easily when going from ~64B to ~2KB.
My main point is that your approach does not follow what I could
witness so far in the UAPI. You have not yet answered this point.
> In fact I don't know any SocketCAN application that uses malloc for CAN
> frames which likely introduces lags and affects the performance.
This argument is invalid. You can malloc() once at the beginning of
the program and reuse it for the entire lifetime (please forget my
previous code snippet which was not meant to be performant).
There is no way that a single malloc can introduce noticeable lags
compared to a static allocation.
> >>>
> >>> 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
