On 7/13/22 11:40, Eelco Chaudron wrote:
> Sorry Mike/Roi, but for some reason, stgit removed you from the CC list while 
> sending out this patch.
> 
> //Eelco
> 
> 
> On 13 Jul 2022, at 10:19, Eelco Chaudron wrote:
> 
>> This series adds support for the datapath action check_pkt_len for
>> TC offload. It also includes some offload self-tests.
>>
>> I had chat offline with Mike, and he is ok with keeping his ACK
>> on the changes in patch 3, however, I did remove Roi's.
>>
>> v2:
>>   - Add ACKs
>>   - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's
>>   - Added section in the NEWS document
>> v3:
>>   - Add ACKs
>>   - Was using TCA_CSUM_PARMS instead of TCA_POLICE_TBF
>> v4:
>>   - Add ACKs
>>   - Use the existing dbinit-aux-args argument, rather than
>>     creating a new pre-vswitchd command option.
>>   - Removed ACKs for patch 4/5
>> v5:
>>   - Sync with upstream branch.
>>
>> Eelco Chaudron (5):
>>       netdev-offload-tc: Move flow_put action handling to isolated function.
>>       netdev-offload-tc: Move flower_to_match action handling to isolated 
>> function.
>>       netdev-offload-tc: Handle check_pkt_len datapath action.
>>       system-offloads-traffic: Properly initialize offload before testing.
>>       tests: Add check_pkt_len action test to system-offload-traffic.
>>
>>
>>  NEWS                             |   1 +
>>  lib/netdev-offload-tc.c          | 885 +++++++++++++++++++------------
>>  lib/ofp-actions.c                |   2 +-
>>  lib/tc.c                         | 443 ++++++++++++++--
>>  lib/tc.h                         |  22 +-
>>  tests/ofproto-macros.at          |   3 +-
>>  tests/system-kmod-macros.at      |   4 +-
>>  tests/system-offloads-traffic.at | 432 ++++++++++++++-
>>  tests/system-tso-macros.at       |   4 +-
>>  tests/system-userspace-macros.at |   4 +-
>>  10 files changed, 1391 insertions(+), 409 deletions(-)

Hi, Eelco.  Thanks for the rebase!

The series looks fine too me and tests are working.
I made a few cosmetic changes and fixed a couple of small issues
along the review.  I can apply the patch set, if the changes looks
good to you:

1. s/_parse_tc_flower_to_actions/parse_tc_flower_to_actions__/
   This is documented in the coding style.

2. s/AT_CHECK_ACTIONS/OVS_CHECK_ACTIONS/
   We shouldn't create new macros with AT_ prefix, these are for
   native aototest macros.

3. Following change:

diff --git a/lib/tc.c b/lib/tc.c
index 090e91671..25e66b5c5 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2816,7 +2816,7 @@ rewrite_pedits_need_csum_update(struct tc_action *action)
                      &first_word_mask, &mask, &data);
 
         for (j = 0; j < cnt; j++,  mask++) {
-            ovs_be32 mask_word = *mask;
+            ovs_be32 mask_word = get_unaligned_be32(mask);
 
             if (j == 0) {
                 mask_word &= first_word_mask;
---
   See 
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
   for details.

4. Fixed tso and userspace test macros as they are not fully correct:

-   _OVS_VSWITCHD_START([--disable-system] [] [$3])
+   _OVS_VSWITCHD_START([--disable-system], [$3])


-   _OVS_VSWITCHD_START([--disable-system] [$3])
+   _OVS_VSWITCHD_START([--disable-system], [$3])


What do you think?

Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to