On 15.06.2017 12:03, Stokes, Ian wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:[email protected]]
>> Sent: Thursday, June 15, 2017 9:45 AM
>> To: Stokes, Ian <[email protected]>; [email protected]; Darrell Ball
>> <[email protected]>
>> Cc: Heetae Ahn <[email protected]>; Daniele Di Proietto
>> <[email protected]>; Ben Pfaff <[email protected]>; Pravin Shelar
>> <[email protected]>; Loftus, Ciara <[email protected]>; Kevin Traynor
>> <[email protected]>
>> Subject: Re: [PATCH v2 2/3] dpif-netdev: Avoid port's reconfiguration on
>> pmd-cpu-mask changes.
>>
>> 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 <[email protected]>
>>>> ---
>>>> 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.
>
> Sure, they are not direct concerns as they could be reproduced on master as
> is, but moving to using core count can potentially cause the number of txqs
> requests to be larger than what's supported, for example with hyper threading
> a system can appear with 64 cores & will require 65 txqs queues, depending on
> the DPDK device and mode this can increase the chance of requesting more txqs
> than the device supports (instead of the current model of requesting the
> number of txqs as the number polling threads).
>
> Either way the code is in place to handle such a situation so I think it's
> fine.
>>
>>>> /* 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.
>
> gcc version 6.3.1 20161221 (Red Hat 6.3.1-1) (GCC)
Ok. I'm going to make v3 with additional parentheses and maybe enhanced
comments in the first patch.
If you don't mind I'll add you to Tested-by tag in v3, since there will
only be minor code change.
>>
>>>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
