Re: [PATCH] rpmsg: char: Use preallocated SKBs.

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 12/6/22 15:40, Piotr Wojtaszczyk wrote:
> Hi Arnaud,
> 
> On Tue, Dec 6, 2022 at 1:54 PM Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com
> <mailto:arnaud.pouliquen@foss.st.com>> 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