On 12/6/22 15:40, Piotr Wojtaszczyk wrote: > Hi Arnaud, > > On Tue, Dec 6, 2022 at 1:54 PM Arnaud POULIQUEN <arnaud.pouliquen@xxxxxxxxxxx > <mailto:arnaud.pouliquen@xxxxxxxxxxx>> wrote: >> On 12/6/22 09:50, Piotr Wojtaszczyk wrote: >> > On a message reception copy the message to a SKB taken from preallocated >> > pool instead of allocating a new SKB each time. >> > During high rpmsg traffic this reduces consumed CPU time noticeably. >> >> Do you have any metrics to share? > Tested on 1GHZ single core ARM Cortex-A55 (64bit), virtio backend. > Ping-pong pair messages (receive + send) every 125us reduced cpu load from 7% to 6%. > >> > +static inline >> > +struct sk_buff *rpmsg_eptdev_get_skb(struct rpmsg_eptdev *eptdev) >> > +{ >> > + struct sk_buff *skb; >> > + >> > + skb = skb_dequeue(&eptdev->skb_pool); >> > + if (!skb) >> > + skb = alloc_skb(MAX_RPMSG_BUF_SIZE, GFP_ATOMIC); >> >> The "get_mtu" endpoint ops should be used here. >> But in any case this works for the virtio backend which defines get_mtu ops >> (asit define the MAX_RPMSG_BUF_SIZE), but not for other backend such as glink. >> Your proposal needs to be compatible with the legacy. >> >> Here is a proposal: >> >> static struct >> sk_buff *rpmsg_eptdev_get_skb(struct rpmsg_eptdev *eptdev, int len) >> { >> struct sk_buff *skb; >> >> if (eptdev->ept->ops->get_mtu) { >> skb = skb_dequeue(&eptdev->skb_pool); >> if (!skb) >> skb = alloc_skb(eptdev->ept->ops->get_mtu(eptdev->ept), >> GFP_ATOMIC); >> } else { >> alloc_skb (len); >> } >> } > The received messages can have different lengths, if we try to reuse skb > which was allocated for smaller a message previously, that is a problem, isn't it? > I went for the worst case scenario in the virtio backend. The get_mtu give you the max transmit unit which should be > len, but some checks can be added Regards, Arnaud > > >> > @@ -126,6 +161,18 @@ static int rpmsg_eptdev_open(struct inode *inode, > struct file *filp) >> > struct rpmsg_endpoint *ept; >> > struct rpmsg_device *rpdev = eptdev->rpdev; >> > struct device *dev = &eptdev->dev; >> > + struct sk_buff *skb; >> > + int i; >> > + >> > + /* Preallocate 8 SKBs */ >> > + for (i = 0; i < 8; i++) { >> >> Do you need to preallocate them? >> during runtime, it will try to reuse SKBs of the skb_pool and if no more >> available it will create a new one. >> This would also help to solve the issue of using MAX_RPMSG_BUF_SIZE > Agree, we can allocate SKBs at run time if needed. I thought it would be better > to start with some SKBs but I think now it's an overkill. > > > -- > Piotr Wojtaszczyk > Timesys
