On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 19.07.22 16:11, Vincent Mailhol wrote:
> > On Tue. 19 Jul. 2022, 20:44, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> >> Extend the CAN device driver infrastructure to handle CAN XL frames.
> >> This especially addresses the increased data length which is extended
> >> to uint16 for CAN XL.
> >>
> >> Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> >> ---
> >> drivers/net/can/dev/rx-offload.c | 2 +-
> >> drivers/net/can/dev/skb.c | 49 ++++++++++++++++++++++++++------
> >> include/linux/can/skb.h | 5 +++-
> >> 3 files changed, 45 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> >> index a32a01c172d4..8505e547e922 100644
> >> --- a/drivers/net/can/dev/rx-offload.c
> >> +++ b/drivers/net/can/dev/rx-offload.c
> >> @@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
> >> unsigned int *frame_len_ptr)
> >> {
> >> struct net_device *dev = offload->dev;
> >> struct net_device_stats *stats = &dev->stats;
> >> struct sk_buff *skb;
> >> - u8 len;
> >> + unsigned int len;
> >> int err;
> >>
> >> skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
> >> if (!skb)
> >> return 0;
> >> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> >> index 8bb62dd864c8..8531e0c39d1c 100644
> >> --- a/drivers/net/can/dev/skb.c
> >> +++ b/drivers/net/can/dev/skb.c
> >> @@ -53,11 +53,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> >> BUG_ON(idx >= priv->echo_skb_max);
> >>
> >> /* check flag whether this packet has to be looped back */
> >> if (!(dev->flags & IFF_ECHO) ||
> >> (skb->protocol != htons(ETH_P_CAN) &&
> >> - skb->protocol != htons(ETH_P_CANFD))) {
> >> + skb->protocol != htons(ETH_P_CANFD) &&
> >> + skb->protocol != htons(ETH_P_CANXL))) {
> >> kfree_skb(skb);
> >> return 0;
> >> }
> >>
> >> if (!priv->echo_skb[idx]) {
> >> @@ -86,12 +87,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(can_put_echo_skb);
> >>
> >> struct sk_buff *
> >> -__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> >> - unsigned int *frame_len_ptr)
> >> +__can_get_echo_skb(struct net_device *dev, unsigned int idx,
> >> + unsigned int *len_ptr, unsigned int *frame_len_ptr)
> >> {
> >> struct can_priv *priv = netdev_priv(dev);
> >>
> >> if (idx >= priv->echo_skb_max) {
> >> netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
> >> @@ -103,17 +104,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> >> /* Using "struct canfd_frame::len" for the frame
> >> * length is supported on both CAN and CANFD frames.
> >> */
> >> struct sk_buff *skb = priv->echo_skb[idx];
> >> struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
> >> - struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> >>
> >> /* get the real payload length for netdev statistics */
> >> - if (cf->can_id & CAN_RTR_FLAG)
> >> - *len_ptr = 0;
> >> - else
> >> - *len_ptr = cf->len;
> >> + *len_ptr = can_skb_get_data_len(skb);
> >>
> >> if (frame_len_ptr)
> >> *frame_len_ptr = can_skb_priv->frame_len;
> >>
> >> priv->echo_skb[idx] = NULL;
> >> @@ -139,11 +136,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> >> */
> >> unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
> >> unsigned int *frame_len_ptr)
> >> {
> >> struct sk_buff *skb;
> >> - u8 len;
> >> + unsigned int len;
> >>
> >> skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
> >> if (!skb)
> >> return 0;
> >>
> >> @@ -244,10 +241,41 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> >>
> >> return skb;
> >> }
> >> EXPORT_SYMBOL_GPL(alloc_canfd_skb);
> >>
> >> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
> >> + struct canxl_frame **cfx)
> >> +{
> >> + struct sk_buff *skb;
> >> +
> >> + skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
> >> + sizeof(struct canxl_frame));
> >
> > I am confused. In this message:
> > https://lore.kernel.org/linux-can/3dcc85f8-2c02-dfe5-de23-d022f3fc56be@xxxxxxxxxxxx/
> > you said that you "vote for selecting the 'truncated' option only"
> > (which is OK with me). But here, you are allocating the full size. If
> > we always want truncated frames, shouldn't we allocate the exact size
> > frame length here?
>
> 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.
> ---8<---
>
> It is just easier and safe.
>
> (..)
>
> >> @@ -302,10 +330,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> >> goto inval_skb;
> >> } else if (skb->protocol == htons(ETH_P_CANFD)) {
> >> if (unlikely(skb->len != CANFD_MTU ||
> >> cfd->len > CANFD_MAX_DLEN))
> >> goto inval_skb;
> >> + } else if (skb->protocol == htons(ETH_P_CANXL)) {
> >
> > Nitpick but with the growing list, I would see a switch
> > (skb->protocol) rather than the cascade of if.
> >
> >> + if (unlikely(!can_is_canxl_skb(skb)))
> >> + goto inval_skb;
> >> } else {
> >
> > default:
> >
> >> goto inval_skb;
> >> }
>
> Yes. Good idea!
>
> Will change that.
Maybe even better:
switch(ntohs(skb->protocol)) {
case ETH_P_CAN:
/* ... */
case ETH_P_CANFD:
/* ... */
case ETH_P_CANXL:
/* ... */
default:
/* ... */
}
