Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/11/2017 6:25 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, January 11, 2017 5:48 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Stephen Hemminger <stephen@networkplumber.org>
>> Cc: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>; olivier.matz@6wind.com; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs
>>
>> On 1/11/2017 5:43 PM, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>>> Sent: Wednesday, January 11, 2017 5:36 PM
>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>>>> Cc: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>; olivier.matz@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs
>>>>
>>>> On Wed, 11 Jan 2017 17:28:21 +0000
>>>> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>>>>>> Sent: Wednesday, January 11, 2017 4:18 PM
>>>>>> To: Sergey Vyazmitinov <s.vyazmitinov@brain4net.com>
>>>>>> Cc: olivier.matz@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org
>>>>>> Subject: Re: [dpdk-dev] [PATCH] kni: use bulk functions to allocate and free mbufs
>>>>>>
>>>>>> On Fri, 30 Dec 2016 04:50:16 +0700
>>>>>> Sergey Vyazmitinov <s.vyazmitinov@brain4net.com> wrote:
>>>>>>
>>>>>>>  /**
>>>>>>> + * Free n packets mbuf back into its original mempool.
>>>>>>> + *
>>>>>>> + * Free each mbuf, and all its segments in case of chained buffers. Each
>>>>>>> + * segment is added back into its original mempool.
>>>>>>> + *
>>>>>>> + * @param mp
>>>>>>> + *   The packets mempool.
>>>>>>> + * @param mbufs
>>>>>>> + *   The packets mbufs array to be freed.
>>>>>>> + * @param n
>>>>>>> + *   Number of packets.
>>>>>>> + */
>>>>>>> +static inline void rte_pktmbuf_free_bulk(struct rte_mempool *mp,
>>>>>>> +		struct rte_mbuf **mbufs, unsigned n)
>>>>>>> +{
>>>>>>> +	struct rte_mbuf *mbuf, *m_next;
>>>>>>> +	unsigned i;
>>>>>>> +	for (i = 0; i < n; ++i) {
>>>>>>> +		mbuf = mbufs[i];
>>>>>>> +		__rte_mbuf_sanity_check(mbuf, 1);
>>>>>>> +
>>>>>>> +		mbuf = mbuf->next;
>>>>>>> +		while (mbuf != NULL) {
>>>>>>> +			m_next = mbuf->next;
>>>>>>> +			rte_pktmbuf_free_seg(mbuf);
>>>>>>> +			mbuf = m_next;
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +	rte_mempool_put_bulk(mp, (void * const *)mbufs, n);
>>>>>>> +}
>>>>>>
>>>>>> The mbufs may come from different pools. You need to handle that.
>>>>>
>>>>> I suppose both stituations are possible:
>>>>> 1) user knows off-hand that all mbufs in the group are from the same mempool
>>>>> 2) user can't guarantee that all mbufs in the group are from same mempool.
>>>>>
>>>>> As I understand that patch is for case 1) only.
>>>>> For 2) it could be a separate function and separate patch.
>>>>>
>>>>> Konstantin
>>>>>
>>>>>
>>>>
>>>> Please don't make unnecessary assumptions in pursuit of minor optimizations.
>>>
>>> I don't suggest to make *any* assumptions.
>>> What I am saying we  can have 2 functions for two different cases.
>>
>> kni_free_mbufs() is static function. Even user knows if all mbufs are
>> some same pool or not, can't pass this information to the free function.
>>
>> Of course this information can be passed via new API, or as an update to
>> exiting API, but I think it is better to update free function to cover
>> both cases instead of getting this information from user.
> 
> I suppose misunderstanding came from the fact that kni_free_mbufs()
> is modified to use rte_pktmbuf_free_bulk(mp, mbufs, n).
> I am not talking about kni part of the patch
> (to be honest I didn't pay much attention to it).
> What I am saying there are many situations when user knows off-hand
> that all  mbufs in that group are from the same mempool and such
> function will be useful too.

> BTW, for my own curiosity, how it could happen with KNI that 
> kni_fifo_get() would return mbufs not from kni->pktmbuf_pool
> (I am not really familiar with KNI and its use-cases)?

It gets packets from free queue:
kni_fifo_get(kni->free_q, ...)

DPDK app may send a mbuf (from any pool, like another port's mempool) to
kernel, kernel puts buf back to kni->free_q when done with it.

> Konstantin
> 
>>
>>> Obviously we'll have to document it properly.
>>> Konstantin
>>>
>>>> It is trivial to write a correct free bulk that handles pool changing.
>>>> Also the free_seg could be bulked as well.
>