From: "Etelson, Gregory" <getelson@nvidia.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: dev@dpdk.org, mkashani@nvidia.com, <thomas@monjalon.net>,
"Aman Singh" <aman.deep.singh@intel.com>,
"Yuying Zhang" <yuying.zhang@intel.com>
Subject: Re: [PATCH] testpmd: add hairpin-map parameter
Date: Wed, 30 Oct 2024 09:08:46 +0200 (IST) [thread overview]
Message-ID: <1fd13eb4-5051-f562-d72d-a277f5d7c91@nvidia.com> (raw)
In-Reply-To: <20241029192945.4cfd7d73@hermes.local>
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
next prev parent reply other threads:[~2024-10-30 7:09 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-28 13:39 Gregory Etelson
2024-10-30 2:29 ` Stephen Hemminger
2024-10-30 7:08 ` Etelson, Gregory [this message]
-- strict thread matches above, loose matches on Subject: below --
2023-09-19 10:10 Gregory Etelson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1fd13eb4-5051-f562-d72d-a277f5d7c91@nvidia.com \
--to=getelson@nvidia.com \
--cc=aman.deep.singh@intel.com \
--cc=dev@dpdk.org \
--cc=mkashani@nvidia.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).