Hi Oliver, On Thu, May 12, 2022 at 06:54:46PM +0200, Oliver Hartkopp wrote: > Hi Oleksij, > > On 12.05.22 14:59, Oleksij Rempel wrote: > > Add CAN specific skb extension support and add first currently needed > > local_origin variable. > > > > On the CAN stack we push same skb data in different direction depending > > on the interface type: > > - to the HW egress and at same time back to the stack as echo > > - over virtual vcan/vxcan interfaces as egress on one side and ingress on other > > side of the vxcan tunnel. > > We can't use skb->sk as marker of the origin, because not all packets > > not all packets with local_origin are assigned to some socket. Some of > > them are generate from the kernel, for example like J1939 control messages. > > So, to properly detect flow direction is is better to store this information > > as part of the SKB. > > > > The advantage of using skb_ext is that it is options and extendable > > without affecting other skb users. It can be shared between cloned skbs and > > duplicated only if needed. > > > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> > > Cc: Devid Antonio Filoni <devid.filoni@xxxxxxxxxxxxxxxxxxxxx> > > --- > > changes v2: > > - migrate it to SKB_EXT > > The use of SKB_EXT seems to be very costly to just store a boolean value. > > What I could see from some of the other SKB_EXT users this extension (which > performs alloc & COW) is used in special circumstances. > > With your suggestion this additional effort is needed for every CAN related > skb. > > So at least for this use-case extending struct can_skb_priv seems to be more > efficient. > > https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L44 > > We might get into problems with PF_PACKET sockets when extending the > can_skb_priv length beyond HH_DATA_MOD, see: > > https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L99 > > But for now I'm not sure that SKB_EXT isn't too heavy to store that single > flag. Yes, I was thinking about potential overkill for, currently, just one bit of storage. But here is my motivation: CAN frameworks is currently using two ways to store metaadata (expecpt of SKB): 1. skb->cb. This variant is not extendable and can be used only insight of one driver. 2. can_skb_priv as part of skb->data->head. Is potentially extendable but we will need to use skb_copy instead of skb_clone. Because we can't modify head for clone only. IMO, this will add potentially more overhead than SKB_EXT. In long term, as soon as we will need to extend can specific meta data, we will have same situation: it will be not big enough to migrate to SKB_EXT. Maybe we need to move can_skb_priv to SKB_EXT as well? > > > > drivers/net/can/vxcan.c | 4 ++++ > > include/linux/can/skb.h | 4 ++++ > > include/linux/skbuff.h | 3 +++ > > net/can/Kconfig | 1 + > > net/can/af_can.c | 5 +++++ > > net/can/raw.c | 10 ++++++++-- > > net/core/skbuff.c | 7 +++++++ > > 7 files changed, 32 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c > > index 577a80300514..93701a698008 100644 > > --- a/drivers/net/can/vxcan.c > > +++ b/drivers/net/can/vxcan.c > > @@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) > > struct net_device *peer; > > struct canfd_frame *cfd = (struct canfd_frame *)oskb->data; > > struct net_device_stats *peerstats, *srcstats = &dev->stats; > > + struct can_skb_ext *can_ext; > > struct sk_buff *skb; > > u8 len; > > @@ -66,6 +67,9 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) > > skb->pkt_type = PACKET_BROADCAST; > > skb->dev = peer; > > skb->ip_summed = CHECKSUM_UNNECESSARY; > > + can_ext = skb_ext_add(skb, SKB_EXT_CAN); > > + if (can_ext) > > + can_ext->local_origin = false; > > len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len; > > if (netif_rx(skb) == NET_RX_SUCCESS) { > > diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h > > index fdb22b00674a..401b08890d74 100644 > > --- a/include/linux/can/skb.h > > +++ b/include/linux/can/skb.h > > @@ -55,6 +55,10 @@ struct can_skb_priv { > > struct can_frame cf[]; > > }; > > +struct can_skb_ext { > > + bool local_origin; > > +}; > > + > > static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb) > > { > > return (struct can_skb_priv *)(skb->head); > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index 3270cb72e4d8..d39e70e5f7f2 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -4563,6 +4563,9 @@ enum skb_ext_id { > > #endif > > #if IS_ENABLED(CONFIG_MCTP_FLOWS) > > SKB_EXT_MCTP, > > +#endif > > +#if IS_ENABLED(CONFIG_CAN) > > + SKB_EXT_CAN, > > #endif > > SKB_EXT_NUM, /* must be last */ > > }; > > diff --git a/net/can/Kconfig b/net/can/Kconfig > > index a9ac5ffab286..eb826e3771fe 100644 > > --- a/net/can/Kconfig > > +++ b/net/can/Kconfig > > @@ -5,6 +5,7 @@ > > menuconfig CAN > > tristate "CAN bus subsystem support" > > + select SKB_EXTENSIONS > > help > > Controller Area Network (CAN) is a slow (up to 1Mbit/s) serial > > communications protocol. Development of the CAN bus started in > > diff --git a/net/can/af_can.c b/net/can/af_can.c > > index 1fb49d51b25d..329c540d3ddf 100644 > > --- a/net/can/af_can.c > > +++ b/net/can/af_can.c > > @@ -201,6 +201,7 @@ int can_send(struct sk_buff *skb, int loop) > > struct sk_buff *newskb = NULL; > > struct canfd_frame *cfd = (struct canfd_frame *)skb->data; > > struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats; > > + struct can_skb_ext *can_ext; > > int err = -EINVAL; > > if (skb->len == CAN_MTU) { > > @@ -240,6 +241,10 @@ int can_send(struct sk_buff *skb, int loop) > > skb_reset_network_header(skb); > > skb_reset_transport_header(skb); > > + can_ext = skb_ext_add(skb, SKB_EXT_CAN); > > + if (can_ext) > > + can_ext->local_origin = true; > > + > > if (loop) { > > /* local loopback of sent CAN frames */ > > diff --git a/net/can/raw.c b/net/can/raw.c > > index b7dbb57557f3..cba18cdf017f 100644 > > --- a/net/can/raw.c > > +++ b/net/can/raw.c > > @@ -121,6 +121,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data) > > { > > struct sock *sk = (struct sock *)data; > > struct raw_sock *ro = raw_sk(sk); > > + struct can_skb_ext *can_ext; > > struct sockaddr_can *addr; > > struct sk_buff *skb; > > unsigned int *pflags; > > @@ -173,8 +174,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data) > > /* add CAN specific message flags for raw_recvmsg() */ > > pflags = raw_flags(skb); > > *pflags = 0; > > - if (oskb->sk) > > - *pflags |= MSG_DONTROUTE; > > + > > + can_ext = skb_ext_find(oskb, SKB_EXT_CAN); > > + if (can_ext) { > > + if (can_ext->local_origin) > > + *pflags |= MSG_DONTROUTE; > > + } > > + > > if (oskb->sk == sk) > > *pflags |= MSG_CONFIRM; > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 475183f37891..5a5409ccb767 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -61,6 +61,7 @@ > > #include <linux/if_vlan.h> > > #include <linux/mpls.h> > > #include <linux/kcov.h> > > +#include <linux/can/skb.h> > > #include <net/protocol.h> > > #include <net/dst.h> > > @@ -4338,6 +4339,9 @@ static const u8 skb_ext_type_len[] = { > > #if IS_ENABLED(CONFIG_MCTP_FLOWS) > > [SKB_EXT_MCTP] = SKB_EXT_CHUNKSIZEOF(struct mctp_flow), > > #endif > > +#if IS_ENABLED(CONFIG_CAN) > > + [SKB_EXT_CAN] = SKB_EXT_CHUNKSIZEOF(struct can_skb_ext), > > +#endif > > }; > > static __always_inline unsigned int skb_ext_total_length(void) > > @@ -4357,6 +4361,9 @@ static __always_inline unsigned int skb_ext_total_length(void) > > #endif > > #if IS_ENABLED(CONFIG_MCTP_FLOWS) > > skb_ext_type_len[SKB_EXT_MCTP] + > > +#endif > > +#if IS_ENABLED(CONFIG_CAN) > > + skb_ext_type_len[SKB_EXT_CAN] + > > #endif > > 0; > > } > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
