On 7/11/22 17:22, Finn, Emma wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets <[email protected]>
>> Sent: Monday 11 July 2022 10:48
>> To: Finn, Emma <[email protected]>; Eelco Chaudron
>> <[email protected]>
>> Cc: [email protected]; [email protected]; Van Haaren, Harry
>> <[email protected]>; Amber, Kumar <[email protected]>;
>> Stokes, Ian <[email protected]>
>> Subject: Re: [v8 00/10] Actions Infrastructure + Optimizations
>>
>> On 7/11/22 11:44, Finn, Emma wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Eelco Chaudron <[email protected]>
>>>> Sent: Friday 8 July 2022 10:53
>>>> To: Finn, Emma <[email protected]>
>>>> Cc: [email protected]; Van Haaren, Harry
>>>> <[email protected]>; Amber, Kumar
>> <[email protected]>;
>>>> Stokes, Ian <[email protected]>; [email protected]
>>>> Subject: Re: [v8 00/10] Actions Infrastructure + Optimizations
>>>>
>>>>
>>>>
>>>> On 7 Jul 2022, at 17:38, Emma Finn wrote:
>>>>
>>>>> This patchset introduces actions infrastructure changes which allows
>>>>> the user to choose between different action implementations based on
>>>>> CPU ISA by using different commands.  The infrastructure also
>>>>> provides a way to check the correctness of the ISA optimized action
>>>>> version against the scalar version.
>>>>>
>>>>> This series also introduces optimized versions of the following
>>>>> actions:
>>>>>  - push_vlan
>>>>>  - pop_vlan
>>>>>  - set_masked eth
>>>>>  - set_masked ipv4
>>>>>
>>>>> Below is a table indicating some relative performance benefits for
>>>>> these actions.
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | Actions                                       | Scalar with series| AVX 
>>>>> with series |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | mod_dl_dst                                    | 1.01x             | 
>>>>> 1.13x           |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | push_vlan                                     | 1.01x             | 
>>>>> 1.10x           |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | strip_vlan                                    | 1.01x             | 
>>>>> 1.11x           |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | mod_ipv4 1 x field                            | 1.01x             | 
>>>>> 1.02x           |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | mod_ipv4 4 x fields                           | 1.01x             | 
>>>>> 1.21x           |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>>> | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.01x             | 
>>>>> 1.36x
>> |
>>>>> +-----------------------------------------------+-------------------+-----------------
>> +
>>>> Hi Emma,
>>>>
>>>> Thanks for the v8! I’m planning to review this next week, however,
>>>> there are some ongoing escalations and people that are on PTO, so no
>>>> promise ;)
>>>>
>>>> Also, I noticed a build failure by the robot you might want to
>>>> investigate in the meantime.
>>>>
>>>> Cheers,
>>>>
>>>> Eelco
>>>>
>>>>
>>> Hi Eelco,
>>>
>>> I have root caused the build failure on patch 6 of this series and I am 
>>> going
>> to send a v9 with this fix today. There will be no functional changes between
>> v8 and v9, the fix is just moving the avx512 probe and init handling from
>> patch 6 to patch 7 in the series.
>>>
>>> Thanks,
>>> Emma
>>>
>>>>> ---
>>>>> v8:
>>>>> - First patch changing unit tests has been removed from series.
>>>>> - AVX checksum implementation has been reworked.
>>>>> - Dependency on userspace datapath has been resolved.
>>
>> Please, also revisit this item, as what's written here doesn't match the 
>> reality.
>> i.e.:
>>
>>   lib/dpif-netdev.c                   |   5 +
>>
>> It's still there ^^^
>>
>>
>> Best regards, Ilya Maximets.
> 
> Hi Ilya,
> 
> Suggestion is that the init function for the actions be moved to 
> bridge_init() in vswitchd/bridge.c. This way it will be datapath agnostic and 
> I also see a list of init functions here too.

bridge_init() seems to be a good place, but I didn't test that.

> 
> If that's not OK, please indicate exactly how you see it integrating, as the 
> integration deadline is on Friday and I plan to send next revision within the 
> next day with the above suggestion implemented.
> 
> Thanks, 
> Emma
> 
> 
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to