[ovs-dev] [v12 06/11] dpif-netdev: Add packet count and core id paramters for study

Eelco Chaudron echaudro at redhat.com
Thu Jul 15 09:47:07 UTC 2021



On 15 Jul 2021, at 11:24, Van Haaren, Harry wrote:

> Hi Eelco,
>
> Thanks for review. Updates with [hvh] prefix below. In general I've fixed-up the suggestions,
> with the exception of 1 that I didn't understand or think actually worked.

ACK, will comment on the one remaining item.


> I'll ask Amber to integrate the changes below into the patchset, next version to ML soon.
>
> Regards, -Harry
>

<SNIP>

>
> if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, mfex_name) == 0)
>
> [hvh] It was previously used inside the .c which declares that array. That array isn't available in dpif-netdev.c (nor should it be), so the above is the pragmatic solution. Study is an exception case in this code snippet anyway as it accepts extra args that the other mfex impls don’t.

ACK, you are right! Guess al we need here is to move to strcmp to avoid partial matches.

<SNIP>

> Below you break again with the idea of doing every parameter in a separate case?
> I think it should be something like (not tested, and assuming you removed mfex_name_is_study and mfex_name_parsed):
>
> [hvh] I'm not following the suggestion here, sorry. A parameter after mfex_name is only valid if mfex_name == "study". The implementation today checks that there is another argument IFF mfex impl == study, and consumes it as "study count" in that case. That's exactly we want?
>
> [hvh] study_count = 0; as per suggestion above, so the else(… && study_count) will always fail, and never execute? Handling argc is the best way to do what we want, I see no issue with it, so leaving as is.
>

My bad should have been !study_count

>           } else if ((mfex_name && study_count) {
> ·
>
> o              if (argc >= 2) {
>
>      *

Reading my explanation again, it’s not clear :)

The command line has the following syntax:

dpif-netdev/miniflow-parser-set [-pmd core] miniflow_implementation_name [study_pkt_cnt]

The goal of the while(argc < 1) loop was to process one argument at the time.

But your current code does the following:

while(argc < 1) {

   if pmd:
      process pmd:
   else name
      process name:
        if study_count
          process study_count
   else
      Error
}

As you can see, the process study_count is at the wrong level.
My suggestion was to move it to the same level. Something like this:

while(argc < 1) {

   if !strcmp(argv[1], "-pmd") && pmd_thread_to_change != NON_PMD_CORE_ID:
      process pmd
   else !mfex_name
      process name
   else if (mfex_name && !study_count):
      process study_count
   else
      Error
}

<SNIP>



More information about the dev mailing list