[ovs-dev] [PATCH v1] dpdk: Support both shared and per port mempools.

Kevin Traynor ktraynor at redhat.com
Tue Jun 26 19:46:41 UTC 2018


On 06/26/2018 07:59 PM, Ian Stokes wrote:
>>
>>>   -/* Tries to allocate a new mempool - or re-use an existing one where
>>> - * appropriate - on requested_socket_id with a size determined by
>>> - * requested_mtu and requested Rx/Tx queues.
>>> - * On success - or when re-using an existing mempool - the new
>>> configuration
>>> - * will be applied.
>>> +/* Depending on the memory model being used this function tries
>>
>> tries to
>>
>>> + * identify and reuse an existing mempool or tries to allocate new
>>> + * mempool on requested_socket_id with mbuf size corresponding to
>>> + * requested_mtu. On success new configuration will be applied.
>>>    * On error, device will be left unchanged. */
>>>   static int
>>>   netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
>>>       OVS_REQUIRES(dev->mutex)
>>>   {
>>>       uint32_t buf_size = dpdk_buf_size(dev->requested_mtu);
>>> -    struct rte_mempool *mp;
>>> +    struct dpdk_mp *dmp;
>>>       int ret = 0;
>>> +    bool per_port_mp = dpdk_per_port_memory();
>>>   -    dpdk_mp_sweep();
>>> +    /* With shared mempools we do not need to configure a mempool if
>>> the MTU
>>> +     * and socket ID have not changed, the previous configuration is
>>> still
>>> +     * valid so return 0 */
>>> +    if (!per_port_mp && dev->mtu == dev->requested_mtu
>>> +        && dev->socket_id == dev->requested_socket_id) {
>>> +        return ret;
>>> +    }
>>>   -    mp = dpdk_mp_create(dev, FRAME_LEN_TO_MTU(buf_size));
>>> -    if (!mp) {
>>> +    dmp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size), per_port_mp);
>>> +    if (!dmp) {
>>>           VLOG_ERR("Failed to create memory pool for netdev "
>>>                    "%s, with MTU %d on socket %d: %s\n",
>>>                    dev->up.name, dev->requested_mtu,
>>> dev->requested_socket_id,
>>>                    rte_strerror(rte_errno));
>>> -        ret = rte_errno;
>>> +        return rte_errno;
>>
>> No need to return here and have the else below. You could set ret or
>> remove the else.
> 
> Sure I'll just set ret = rte_errno and return below.
>>
>>>       } else {
>>> -        /* If a new MTU was requested and its rounded value equals
>>> the one
>>> -         * that is currently used, then the existing mempool is
>>> returned. */
>>> -        if (dev->mp != mp) {
>>> -            /* A new mempool was created, release the previous one. */
>>> -            dpdk_mp_release(dev->mp);
>>> -        } else {
>>> -            ret = EEXIST;
>>> +        /* Check for any pre-existing dpdk_mp for the device before
>>> accessing
>>> +         * the associated mempool.
>>> +         */
>>> +        if (dev->dpdk_mp != NULL) {
>>> +            /* A new MTU was requested and its rounded value does not
>>> +             * equal the rounded value of the current mempool,
>>> decrement
>>> +             * the reference count to the devices current dpdk_mp as
>>> its
>>> +             * associated mempool will not be used for this device.
>>> +             */
>>
>> The comment needs updating. You could be reusing the existing mempool,
>> in which case it's still ok to mp_put because you have just incremented
>> it...
> 
> Good catch, will update for the v2.
> 
>>
>> About that, I know it was my comment to remove an unnecessary check here
>> :-/ but i'm wondering if the refcount inc and then dec for the EEXISTS
>> case is unintuitive and it would better the way you had it with set
>> refcount =1 in mp_get, and then have the check for "if (dev->dpdk_mp !=
>> dmp)" before you mp_put. What do you think?
>>
> 
> I think the approach we have at the moment is better because it handles
> the per port model of EEXIST and handles the reuse case for a single
> port for the shared_memory model described below.
> 
> Add a port 'dpdk0' with default MTU 1500. The associated dpdk mempool
> will have a refcount of 1.
> 
> Now set the mtu of 'dpdk0' to 1800. The existing dpdk mempool will be
> reused and incremented in mp_get. refcount will now be 2 but this is
> incorrect as there is only 1 port and it's reusing the same mempool, we
> have to decrement the refcount by 1 for it to be correct.
> 
> The code as is handles that situation. If you checked for "if
> (dev->dpdk_mp != dmp)" you wouldn't decrement in this case.
> 

ok, sounds good.

> Maybe it needs a comment to this effect?
> 
> What are your thoughts?
> 

Yeah, I think an updated comment, with a note for the existing mempool
case will make it clear. thanks.

> Ian
> 
>>> +            dpdk_mp_put(dev>dpdk_mp);
>>
>>
>>
>>>           }
>>> -        dev->mp = mp;
>>> +        dev->dpdk_mp = dmp;
>>>           dev->mtu = dev->requested_mtu;
>>>           dev->socket_id = dev->requested_socket_id;
>>>           dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu); 



More information about the dev mailing list