Hi Ilya,
On 15/03/2016 06:43, "Ilya Maximets" <i.maxim...@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 <diproiet...@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);
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
