On 13 Jul 2022, at 16:49, Ilya Maximets wrote:
> 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?
Thanks for the additional review, and the changes look good to me.
Was not aware of the __ at the end, guess we might need to change some more
existing code ;) I will re-read the coding style as it’s been a while ;)
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
