Re: [PATCH] testpmd: add hairpin-map parameter

["Etelson, Gregory" <getelson@nvidia.com>]

Hello Stephen,

>> +static __rte_always_inline
>> +char *parse_hairpin_map_entry(char *input, char **next)
>> +{
>> +     char *tail = strchr(input, ':');
>> +
>> +     if (!tail)
>> +             return NULL;
>> +     tail[0] = '\0';
>> +     *next = tail + 1;
>> +     return input;
>> +}
>> +
>
> There is no reason to mark this as inline. It is not in fast path.
> Let compiler decide.
>

There will be no parse_hairpin_map_entry() in the next patch version.

>
>> +     head = parse_hairpin_map_entry(next, &next);
>> +     if (!head)
>> +             goto err;
>> +     map->rx_port = atoi(head);
>
> Use strtoul() to allow checking for invalid number.
> Seems like the parsing here is following the parsing code pattern
> that could use strtok_r.

I switched to sscanf().

>
>> +             /* Fail to setup rx queue, return */
>> +             if (port->port_status == RTE_PORT_HANDLING)
>> +                     port->port_status = RTE_PORT_STOPPED;
>> +             else
>> +                     fprintf(stderr,
>> +                             "Port %d can not be set back to stopped\n", pi);
>> +             fprintf(stderr,
>> +                     "Port %d failed to configure hairpin on rxq %u.\n"
>> +                     "Peer port: %u peer txq: %u\n",
>> +                     pi, qi, peer_tx_port, i);
>
> Minor nit, port should be printed with %u since it is unsigned.
> Lots of testpmd code gets this wrong, and compiler doesn't care.
>

fixed.

>
>> +/* Configure the Rx and Tx hairpin queues for the selected port. */
>> +static int
>> +setup_hairpin_queues(portid_t pi, portid_t p_pi, uint16_t cnt_pi)
>> +{
>> +     return !hairpin_multiport_mode ?
>> +            setup_legacy_hairpin_queus(pi, p_pi, cnt_pi) :
>> +            setup_mapped_harpin_queues(pi);
>> +}
>
> It is clearer to use an if statement in this case.
> Also, better to write positive rather than negative logic.
>
>        if (hairpin_multiport_mode)
>                return setup_mapped_hairpin_queues(pi);
>        else
>                return setup_legacy_hairpin_queus(pi, p_pi, cnt_pi);

fixed.

I'll post fixes in the next patch version.

Regards,
Gregory