On 15.06.2017 11:02, Stokes, Ian wrote:
>> Reconfiguration of HW NICs may lead to packet drops.
>> In current model all physical ports will be reconfigured each time number
>> of PMD threads changed. Since we not stopping threads on pmd-cpu-mask
>> changes, this patch will help to further decrease port's downtime by
>> setting the maximum possible number of wanted tx queues to avoid
>> unnecessary reconfigurations.
>
> Hi Ilya,
>
> Thanks for the patch, in testing I can confirm it resolves the issue. A few
> observations and comments below.
Hi Ian. Thanks for testing.
>
>>
>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
>> ---
>> lib/dpif-netdev.c | 26 +++++++++++++++++++++-----
>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 596d133..79db770
>> 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -3453,7 +3453,7 @@ reconfigure_datapath(struct dp_netdev *dp) {
>> struct dp_netdev_pmd_thread *pmd;
>> struct dp_netdev_port *port;
>> - int wanted_txqs;
>> + int needed_txqs, wanted_txqs;
>>
>> dp->last_reconfigure_seq = seq_read(dp->reconfigure_seq);
>>
>> @@ -3461,7 +3461,15 @@ reconfigure_datapath(struct dp_netdev *dp)
>> * on the system and the user configuration. */
>> reconfigure_pmd_threads(dp);
>>
>> - wanted_txqs = cmap_count(&dp->poll_threads);
>> + /* We need 1 Tx queue for each thread to avoid locking, but we will
>> try
>> + * to allocate the maximum possible value to minimize the number of
>> port
>> + * reconfigurations. */
>> + needed_txqs = cmap_count(&dp->poll_threads);
>> + /* (n_cores + 1) is the maximum that we might need to have.
>> + * Additional queue is for non-PMD threads. */
>> + wanted_txqs = ovs_numa_get_n_cores();
>> + ovs_assert(wanted_txqs != OVS_CORE_UNSPEC);
>> + wanted_txqs++;
>
> So just after this line there is a call
>
> HMAP_FOR_EACH (port, node, &dp->ports) {
> netdev_set_tx_multiq(port->netdev, wanted_txqs);
> }
>
> My initial concern with this patch was twofold:
>
> (i) What would happen if the number of cores was greater than the number of
> supported queues.
>
> That's not an issue from what I can see as the requested_txqs will be
> compared to what's supported by the device and the smaller of the two will be
> chosen in the eventual call to dpdk_eth_dev_init():
>
> n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
>
> (ii) What happens when a device reports the incorrect number of max tx
> queues? (can occur depending on the mode of the device).
>
> I think this case is ok as well as it's handled by the existing txq retry
> logic in dpdk_eth_dev_queue_setup().
Actually these concerns are not about this patch. Such situations are possible
on current master. But you're right, both of them handled properly with
existing code according to conventions between dpif-netdev and netdev layers.
>> /* The number of pmd threads might have changed, or a port can be
>> new:
>> * adjust the txqs. */
>> @@ -3474,9 +3482,17 @@ reconfigure_datapath(struct dp_netdev *dp)
>>
>> /* Check for all the ports that need reconfiguration. We cache this
>> in
>> * 'port->reconfigure', because netdev_is_reconf_required() can
>> change at
>> - * any time. */
>> + * any time.
>> + * Also mark for reconfiguration all ports which will likely change
>> their
>> + * 'dynamic_txqs' parameter. It's required to stop using them before
>> + * changing this setting and it's simpler to mark ports here and
>> allow
>> + * 'pmd_remove_stale_ports' to remove them from threads. There will
>> be
>> + * no actual reconfiguration in 'port_reconfigure' because it's
>> + * unnecessary. */
>> HMAP_FOR_EACH (port, node, &dp->ports) {
>> - if (netdev_is_reconf_required(port->netdev)) {
>> + if (netdev_is_reconf_required(port->netdev)
>> + || (port->dynamic_txqs
>> + != netdev_n_txq(port->netdev) < needed_txqs)) {
>
> GCC caught the following
>
> lib/dpif-netdev.c:3448:48: error: suggest parentheses around comparison in
> operand of '!=' [-Werror=parentheses]
> != netdev_n_txq(port->netdev) < needed_txqs)) {
It's unnecessary because equality operators has higher priority than
relational, but I can add parentheses if compiler complains.
What version of GCC you're using? I didn't see such warnings on my
systems.
>> port->need_reconfigure = true;
>> }
>> }
>> @@ -3510,7 +3526,7 @@ reconfigure_datapath(struct dp_netdev *dp)
>> seq_change(dp->port_seq);
>> port_destroy(port);
>> } else {
>> - port->dynamic_txqs = netdev_n_txq(port->netdev) <
>> wanted_txqs;
>> + port->dynamic_txqs = netdev_n_txq(port->netdev) <
>> + needed_txqs;
>> }
>> }
>>
>> --
>> 2.7.4
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
