[ovs-dev] [PATCH v4] netdev-dpdk: Allow configurable queue sizes for 'dpdk' ports

[Daniele Di Proietto]

2016-09-29 3:28 GMT-07:00 Loftus, Ciara <ciara.loftus at intel.com>:

> >
> > Hi Ciara,
> > thanks for the patch, it looks good to me.
> > I only have a minor comment:
> > I'd like the requested values to depend only on the current database
> > state.  With the current patch when a value is invalid (not pow2 or
> bigger
> > than 4096) we keep the previous one.
> > Could you change dpdk_process_queue_size() to return a default value
> > (which can be passed as an argument) when the value from the database is
> > absent or not valid?
> > I guess dpdk_process_queue_size() could return this value directly,
> instead
> > of returning it through a pointer.
>
> Hi Daniele,
>
> Thanks for the review. Can you please clarify your request.
> What do you suggest we assign the return value of process_queues() to? If
> requested_size is to reflect the DB value then I assume not that.
> The validity checks seem pointless in process_queue_size if we are doing
> to set the requested value regardless. If requested_size reflects the DB
> value  I see two options:
>
1. Do the pow2 and size checks in reconfigure, before assigning
> dev->xq_sizes, and only assign if valid.
> 2. Similar to n_rxq, try set up the queue and if it fails, fall back on a
> known good (previous) value. This removes the pow2 etc checks.
>
> Let me know your opinion, or another option if you have it.
>
>
Sorry, I wasn't clear enough.

The checks look good to me, I was thinking about what to do if the checks
fail.

Considering the following scenario:

ovs-vsctl set int dpdk0 options:n_rxq_desc=1024
ovs-vsctl set int dpdk0 options:n_rxq_desc=3000 #Invalid value.

With your patch, after the second ovs-vsctl, the effective rxq_desc will be
1024 (the previous value).  In my opinion it should be
NIC_PORT_DEFAULT_RXQ_SIZE, like it's specified in the documentation:

"If not specified or an incorrect value is specified, 2048 rx descriptors
will be used by default."

How about something like this? (I realized that it's probably easier to use
the pointer and return void, please disregard my previous suggestion)

static void
dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
                                            const char *flag, int default,
int *new_size)
{
    int queue_size = smap_get_int(args, flag, default);

    if (/* queue size is invalid */) {
        queue_size = default;
    }

    if (queue_size != *new_size) {
        reconfigure();
    }
}

netdev_dpdk_set_config()
{
/*... */
dpdk_process_queue_size(/* */, NIC_PORT_DEFAULT_RXQ_SIZE,
&dev->requested_rxq_size);
dpdk_process_queue_size(/* */, NIC_PORT_DEFAULT_TXQ_SIZE,
&dev->requested_txq_size);

/*... */
}

Thanks,

Daniele