>
> 2016-09-29 3:28 GMT-07:00 Loftus, Ciara <ciara.lof...@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)
Thanks for the clarification, makes sense. I've sent a v5 - let me know if I
misinterpreted your request.
Thanks,
Ciara
>
> 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
