Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9dbc355c-2f73-cfb9-4808-fce65d7860e3@foss.st.com>
Date:   Wed, 7 Dec 2022 09:59:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] rpmsg: char: Use preallocated SKBs.
Content-Language: en-US
To:     Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
CC:     Ohad Ben-Cohen <ohad@wizery.com>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Bjorn Andersson <andersson@kernel.org>
References: <20221206085008.25388-1-piotr.wojtaszczyk@timesys.com>
 <0987cae3-0edc-918b-5b23-22f238f22aa2@foss.st.com>
 <CAG+cZ05b9W4e=AhoiAgehW7V7uWTPaBReAcQkTcVQmDsXwUYFw@mail.gmail.com>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Organization: STMicroelectronics
In-Reply-To: <CAG+cZ05b9W4e=AhoiAgehW7V7uWTPaBReAcQkTcVQmDsXwUYFw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk


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