Re: [ovs-dev] [PATCH v5 1/1] dpif-netdev: Add per pmd sleep config.

[David Marchand]

Hello Kevin,

On Fri, Sep 29, 2023 at 2:50 PM Kevin Traynor <ktray...@redhat.com> wrote:
>
> Extend 'pmd-sleep-max' so that individual PMD thread cores
> may have a specified max sleep request value.
>
> Existing behaviour is maintained.
>
> Any PMD thread core without a value will use the global value
> if set or default no sleep.
>
> To set PMD thread cores 8 and 9 to never request a load based sleep
> and all other PMD thread cores to be able to request a max sleep of
> 50 usecs:
>
> $ ovs-vsctl set open_vswitch . other_config:pmd-sleep-max=50,8:0,9:0
>
> To set PMD thread cores 10 and 11 to request a max sleep of 100 usecs
> and all other PMD thread cores to never request a sleep:
>
> $ ovs-vsctl set open_vswitch . other_config:pmd-sleep-max=10:100,11:100
>
> 'pmd-sleep-show' is updated to show the max sleep value for each PMD thread.
>
> Signed-off-by: Kevin Traynor <ktray...@redhat.com>

It is a bit hard to follow the differences with the v3 patches which I
had reviewed previously.
I understand the main differences was on naming / cosmetics changes
requested by Ilya, and updating vswitch.xml.

In any case, this patch still lgtm, with just a comment on vswitch.xml.
Reviewed-by: David Marchand <david.march...@redhat.com>


> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..8dab1cd6e 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -803,7 +803,5 @@
>          </p>
>        </column>
> -      <column name="other_config" key="pmd-sleep-max"
> -              type='{"type": "integer",
> -                     "minInteger": 0, "maxInteger": 10000}'>
> +      <column name="other_config" key="pmd-sleep-max">
>          <p>
>            Specifies the maximum sleep time that will be requested in
> @@ -824,4 +822,32 @@
>            The maximum value is <code>10000 microseconds</code>.
>          </p>
> +        <p>
> +         <code>other_config:pmd-sleep-max=&lt;
> +           pmd-sleep-list&gt;</code>

Do we need a newline here?
This renders as:
              other_config:pmd-sleep-max=< pmd-sleep-list>


I did not notice in the past when reviewing the shared mempool series,
but I see the same pattern with other_config:shared-mempool-config=<
user-shared-mempool-mtu-list>.
So maybe it is an accepted form, but it seems strange to me.


> +        </p>
> +        <p>where</p>
> +        <p>
> +         <ul>
> +           <li>
> +             &lt;pmd-sleep-list&gt; ::= NULL | &lt;non-empty-list&gt;
> +           </li>
> +           <li>
> +             &lt;non-empty-list&gt; ::= &lt;pmd-sleep-value&gt; |
> +                                        &lt;pmd-sleep-value&gt; ,
> +                                        &lt;non-empty-list&gt;
> +           </li>
> +           <li>
> +             &lt;pmd-sleep-value&gt; ::= &lt;global-default-sleep-value&gt; |
> +                                         &lt;pmd-core-sleep-pair&gt;
> +           </li>
> +           <li>
> +             &lt;global-default-sleep-value&gt; ::= &lt;max-sleep-time&gt;
> +           </li>
> +           <li>
> +             &lt;pmd-core-sleep-pair&gt; ::= &lt;core&gt; :
> +                                             &lt;max-sleep-time&gt;
> +           </li>
> +         </ul>
> +        </p>
>        </column>
>        <column name="other_config" key="userspace-tso-enable"
> --
> 2.41.0
>

-- 
David Marchand

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev