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);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
