DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Varghese, Vipin" <vipin.varghese@amd.com>
To: fengchengwen <fengchengwen@huawei.com>, dev@dpdk.org
Cc: ferruh.yigit@amd.com, neerav.parikh@amd.com
Subject: Re: [PATCH v2] app/dma-perf: calrify incorrect NUMA config
Date: Tue, 12 Mar 2024 09:18:00 +0530	[thread overview]
Message-ID: <f2aee508-e3d6-436b-a820-e723bd95cced@amd.com> (raw)
In-Reply-To: <6ee9b198-4419-2016-a05b-4e1711d946a7@huawei.com>

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

<snipped>
>> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
>> index 9b1f58c78c..b6d0dbe4c0 100644
>> --- a/app/test-dma-perf/benchmark.c
>> +++ b/app/test-dma-perf/benchmark.c
>> @@ -311,9 +311,14 @@ setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
>>        uint32_t nr_buf = cfg->nr_buf;
>>
>>        nr_sockets = rte_socket_count();
>> -     if (cfg->src_numa_node >= nr_sockets ||
>> -             cfg->dst_numa_node >= nr_sockets) {
>> -             printf("Error: Source or destination numa exceeds the acture numa nodes.\n");
>> +
>> +     bool isSrcNumaIncorrect = (cfg->src_numa_node >= nr_sockets);
>> +     bool isDstNumaIncorrect = (cfg->dst_numa_node >= nr_sockets);
> The naming style needs to be adjusted, how about
> bool is_src_numa_exceed, is_dst_numa_exceed;

Ok, the naming convention used by me is `CamelCase`. One suggested from 
your end is `snake_case`.

Does DPDK has a constrain it can not use CamelCase.

>
> And predefine the variable at the beginning of function, sort by length, some like:
>
>          bool is_src_numa_exceed, is_dst_numa_exceed;
>          unsigned int buf_size = cfg->buf_size.cur;
>          uint32_t nr_buf = cfg->nr_buf;
>          unsigned int nr_sockets;
>
>          nr_sockets = rte_socket_count();
>          is_src_numa_exceed =
>          is_dst_numa_exceed =
>          if (xxx)
>                  ...
>
>> +
>> +     if (isSrcNumaIncorrect || isDstNumaIncorrect) {
>> +             PRINT_ERR("Error: NUMA config exceeds the actual numa nodes for %s.\n",
>> +                     (isSrcNumaIncorrect && isDstNumaIncorrect) ? "Source & Destination" :
>> +                             (isSrcNumaIncorrect) ? "Source" : "Destination");
> Please don't capitalize the first letter of "Source" and "Destination"

Can you please explain why?


>
> Thanks
>
>>                return -1;
>>        }
>>
>>

[-- Attachment #2: Type: text/html, Size: 2980 bytes --]

  reply	other threads:[~2024-03-12  3:48 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-06 15:02 [PATCH] " Vipin Varghese
2024-03-07 13:19 ` fengchengwen
2024-03-07 16:06   ` Varghese, Vipin
2024-03-11  6:00 ` [PATCH v2] " Vipin Varghese
2024-03-12  2:10   ` fengchengwen
2024-03-12  3:48     ` Varghese, Vipin [this message]
2024-03-13  9:37       ` Konstantin Ananyev
2024-03-20  1:14       ` Varghese, Vipin
2024-03-20  1:40   ` [PATCH v3] " Vipin Varghese

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=f2aee508-e3d6-436b-a820-e723bd95cced@amd.com \
    --to=vipin.varghese@amd.com \
    --cc=dev@dpdk.org \
    --cc=fengchengwen@huawei.com \
    --cc=ferruh.yigit@amd.com \
    --cc=neerav.parikh@amd.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).