[ovs-dev] [PATCH v2 09/11] dpif-netdev: Fix reconfigure_pmd_threads().

Daniele Di Proietto diproiettod at vmware.com
Tue Mar 15 22:14:06 UTC 2016


Hi Ilya,

On 15/03/2016 06:43, "Ilya Maximets" <i.maximets at samsung.com> wrote:

>comment inline.
>
>Best regards, Ilya Maximets.
>
>On 03.03.2016 04:33, Daniele Di Proietto wrote:
>> This commit change reconfigure_pmd_threads() to interact with the ports
>> cmap using RCU semantics (the content of the port structure is not
>> altered while concurrent readers might access it) and to fail more
>> gracefully in case of a set_multiq fail (now we remove the port from the
>> datapath, instead of returning prematurely from the function without
>> restarting the pmd threads).
>> 
>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>> ---
>>  lib/dpif-netdev.c | 79
>>++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 55 insertions(+), 24 deletions(-)
>> 
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 8ed6bcb..0c3673b 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -2591,6 +2591,11 @@ static void
>>  reconfigure_pmd_threads(struct dp_netdev *dp)
>>  {
>>      struct dp_netdev_port *port;
>> +    struct hmapx to_reconfigure = HMAPX_INITIALIZER(&to_reconfigure);
>> +    struct hmapx_node *node;
>> +    bool failed_config = false;
>> +
>> +    ovs_mutex_lock(&dp->port_mutex);
>>  
>>      dp_netdev_destroy_all_pmds(dp);
>>  
>> @@ -2599,33 +2604,59 @@ reconfigure_pmd_threads(struct dp_netdev *dp)
>>          int requested_n_rxq = netdev_requested_n_rxq(netdev);
>>          if (netdev_is_pmd(port->netdev)
>>              && port->latest_requested_n_rxq != requested_n_rxq) {
>> -            int i, err;
>> +            cmap_remove(&dp->ports, &port->node,
>>hash_odp_port(port->port_no));
>> +            hmapx_add(&to_reconfigure, port);
>> +        }
>> +    }
>> +    ovs_mutex_unlock(&dp->port_mutex);
>>  
>> -            /* Closes the existing 'rxq's. */
>> -            for (i = 0; i < port->n_rxq; i++) {
>> -                netdev_rxq_close(port->rxq[i]);
>> -                port->rxq[i] = NULL;
>> -            }
>> -            port->n_rxq = 0;
>> -
>> -            /* Sets the new rx queue config. */
>> -            err = netdev_set_multiq(port->netdev,
>>ovs_numa_get_n_cores() + 1,
>> -                                    requested_n_rxq);
>> -            if (err && (err != EOPNOTSUPP)) {
>> -                VLOG_ERR("Failed to set dpdk interface %s rx_queue to:
>>%u",
>> -                         netdev_get_name(port->netdev),
>> -                         requested_n_rxq);
>> -                return;
>> -            }
>> -            port->latest_requested_n_rxq = requested_n_rxq;
>> -            /* If the set_multiq() above succeeds, reopens the 'rxq's.
>>*/
>> -            port->n_rxq = netdev_n_rxq(port->netdev);
>> -            port->rxq = xrealloc(port->rxq, sizeof *port->rxq *
>>port->n_rxq);
>> -            for (i = 0; i < port->n_rxq; i++) {
>> -                netdev_rxq_open(port->netdev, &port->rxq[i], i);
>> -            }
>> +    /* Waits for the other threads to see the ports removed from the
>>cmap,
>> +     * otherwise we are not allowed to alter them. */
>> +    ovsrcu_synchronize();
>> +
>> +    ovs_mutex_lock(&dp->port_mutex);
>> +    HMAPX_FOR_EACH (node, &to_reconfigure) {
>> +        int requested_n_rxq = netdev_requested_n_rxq(port->netdev);
>
>This will be removed lately but this is a bug in this patch. We should not
>call netdev_requested_n_rxq here. Also, this may lead to segmentation
>fault
>because of invalid 'port'.

You're right, it is also unnecessary, since requested_n_rxq is properly
set below.

I've removed the initialization.

Thanks for spotting this!

>
>> +        int i, err;
>> +
>> +        port = node->data;
>> +        requested_n_rxq = netdev_requested_n_rxq(port->netdev);
>> +        /* Closes the existing 'rxq's. */
>> +        for (i = 0; i < port->n_rxq; i++) {
>> +            netdev_rxq_close(port->rxq[i]);
>> +            port->rxq[i] = NULL;
>> +        }
>> +        port->n_rxq = 0;
>> +
>> +        /* Sets the new rx queue config. */
>> +        err = netdev_set_multiq(port->netdev, ovs_numa_get_n_cores() +
>>1,
>> +                                requested_n_rxq);
>> +        if (err && (err != EOPNOTSUPP)) {
>> +            VLOG_ERR("Failed to set dpdk interface %s rx_queue to: %u",
>> +                     netdev_get_name(port->netdev),
>> +                     requested_n_rxq);
>> +            do_destroy_port(port);
>> +            failed_config = true;
>> +            continue;
>>          }
>> +        port->latest_requested_n_rxq = requested_n_rxq;
>> +        /* If the netdev_reconfigure() above succeeds, reopens the
>>'rxq's and
>> +         * inserts the port back in the cmap, to allow transmitting
>>packets. */
>> +        port->n_rxq = netdev_n_rxq(port->netdev);
>> +        port->rxq = xrealloc(port->rxq, sizeof *port->rxq *
>>port->n_rxq);
>> +        for (i = 0; i < port->n_rxq; i++) {
>> +            netdev_rxq_open(port->netdev, &port->rxq[i], i);
>> +        }
>> +        cmap_insert(&dp->ports, &port->node,
>>hash_port_no(port->port_no));
>> +    }
>> +    ovs_mutex_unlock(&dp->port_mutex);
>> +
>> +    hmapx_destroy(&to_reconfigure);
>> +
>> +    if (failed_config) {
>> +        seq_change(dp->port_seq);
>>      }
>> +
>>      /* Reconfigures the cpu mask. */
>>      ovs_numa_set_cpu_mask(dp->requested_pmd_cmask);
>>      free(dp->pmd_cmask);
>> 




More information about the dev mailing list