On Tue. 23 Jul. 2022 à 03:52, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 22.07.22 18:32, Vincent Mailhol wrote:
> > 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.
>
> Yes. And it is a good argument.
>
> > 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.
>
> I do see a problem with it and definitely have another personal
> preference. So far I did not need dynamic memory to process CAN content
> in CAN applications which is a good (and required) pattern for embedded
> automotive applications and allows to port concepts and code between
> these two worlds.
ACK that for safety application (as of ISO 26262) static declaration
are prefered (and nearly mandatory at ASIL C and D levels) but the
Linux kernel is not designed for safety.
If your kernel uses dynamic memory allocations, then your application
will not be safer (as of ISO 26262) with static declarations.
I do not believe it will be a problem when porting code from an
embedded application to Socket CAN. It is not the same structure, not
the same field names, not the same API so you have to rewrite the code
related to the declaration, initialization and the read()/write()
regardless. The only part which is portable is the processing of
canxl_frame::data and this part could be ported regardless if you use
dynamic or static memory application.
> > 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.
>
> Because of what?
Because of consistency.
> IMO it is a proper follow up of the current CAN_RAW API.
>
> > 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.
>
> There is an ioctl:
>
> https://manpages.debian.org/testing/manpages-de-dev/ioctl_list.2.de.html
>
> 0x4FA44308 SNDCTL_COPR_SENDMSG const struct copr_msg *
> 0x8FA44309 SNDCTL_COPR_RCVMSG struct copr_msg *
>
> include/uapi/linux/soundcard.h
>
> typedef struct copr_msg {
> int len;
> unsigned char data[4000];
> } copr_msg;
OK. So the best example you found was introduced in Linux 1.x (around
end of 1995!) and already violates the Linux coding guidelines by
using typedef.
Ref: https://elixir.bootlin.com/linux/1.3.9/source/include/linux/soundcard.h#L638
Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs
So I guess you just found the exception that proves the rule.
> >> 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.
>
> As described above omitting dynamic memory as a whole is a valid
> pattern. And so far no one needs dynamic memory or has to be encouraged
> to use it.
>
> With the API of truncated CAN XL frames you can do whatever you want
> (even make use of malloc()) on the user space side.
>
> I don't see any real argument to leave the established pattern.
>
> You probably might feel that I don't like dynamic arrays or have some
> problems of making use of it. This is not the case:
>
> include/uapi/linux/can/bcm.h
>
> struct bcm_msg_head {
> __u32 opcode;
> __u32 flags;
> __u32 count;
> struct bcm_timeval ival1, ival2;
> canid_t can_id;
> __u32 nframes;
> struct can_frame frames[0];
> };
>
>
> Which leads to:
>
> linux-can/can-tests/bcm/tst-bcm-filter.c
>
> struct {
> struct bcm_msg_head msg_head;
> struct can_frame frame[4];
> } txmsg, rxmsg;
>
>
> txmsg.msg_head.opcode = RX_SETUP;
> txmsg.msg_head.can_id = 0x042;
> txmsg.msg_head.flags = SETTIMER|RX_FILTER_ID;
> txmsg.msg_head.ival1.tv_sec = 1;
> txmsg.msg_head.ival1.tv_usec = 0;
> txmsg.msg_head.ival2.tv_sec = 0;
> txmsg.msg_head.ival2.tv_usec = 0;
> txmsg.msg_head.nframes = 0;
>
> But this is something different to me than having a struct canxl_frame
> without data - especially as it requires at least one data byte
> (CANXL_MIN_DLEN) to be a valid CAN XL frame.
>
> 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[CANXL_MIN_DLEN];
> };
>
> A truncated CAN XL frame structure with a data section that contains as
> much valid bytes as given in the len element is the exact representation
> of what you will see on the wire.
>
> For some reason you seem to stick on this (even invalid) "UAPI is using
> dynamic arrays" stuff instead of enhancing and improving the established
> handy and safe pattern for CAN frames.
Please define what you mean as "safe". SocketCAN is not safe by ISO
26262 standard. And in a non safety critical context, static memory
allocation is equally as safe as dynamic memory allocation (i.e. both
are not safe): if you have no memory left, an application, even if
using static declaration, will not start.
This debate is going to a dead end. I do not see one of us fully
convincing the other. As I stated before, the data[CANXL_MIN_DLEN] is
not bad and has the big merit of allowing static declaration. While my
preference definitely goes to the flexible array member, I will not
veto your idea. I think it was important to have this debate.
With that said, I leave the final decision to the mailing list. If
other people prefer the data[CANXL_MIN_DLEN] over the flexible array
member, or if no one other than the two of us care, then let's use
your approach!
Yours sincerely,
Vincent Mailhol
