[ovs-dev] [RFC v3 2/5] Add a new OVS action check_pkt_larger

[Numan Siddique]

Hi Ben,

Few comments inline.

Thanks
Numan

On Wed, Feb 13, 2019 at 10:53 PM Numan Siddique <nusiddiq at redhat.com> wrote:

>
> Thanks Ben for providing the review comments.
>
> I will address all the comments.
>
> A couple of comments inline.
>
> Thanks
> Numan
>
>
> On Tue, Feb 12, 2019 at 8:33 AM Ben Pfaff <blp at ovn.org> wrote:
>
>> On Thu, Jan 10, 2019 at 11:30:33PM +0530, nusiddiq at redhat.com wrote:
>> > From: Numan Siddique <nusiddiq at redhat.com>
>> >
>> > This patch adds a new action 'check_pkt_larger' which checks if the
>> > packet is larger than the given size and stores the result in the
>> > destination register.
>>
>> Thanks.
>>
>> This patch renumbers OVS_ACTION_ATTR_CLONE and moves it into the
>> "userspace only" group of datapath actions.  It should not do that.
>> It should also not remove OVS_CLONE_ATTR_EXEC.
>>
>> ofpact_remaining_len() should not return bool.  (This mistake suggests
>> that the case where it should return nonzero was not tested.)
>>
>> I don't understand the check for check_pkt_larger->dst.field in
>> xlate_check_pkt_larger().  The action would be invalid if this was
>> missing.  Such an ofpact should not be translated.
>>
>> The following comment appears incomplete:
>>         /* If datapath doesn't support check_pkt_len action, then set the
>>          * SLOW_ACTION flag so that ..
>>          */
>>
>> odp-util.c has code for formatting the new action, but not for parsing.
>> We need both.
>>
>> The format might be a little too cute.  It might be wise to use
>> something more like check_pkt_len(size=123, gt(...), le(...)).
>>
>> decode_OFPAT_RAW_CHECK_PKT_LARGER() should check that the field
>> specified and the bits in it are valid and usable and return an error if
>> they are not.
>>
>> As is, the OpenFlow action format only allows NXM headers.  All new
>> actions should also support OXM 64-bit experimenter headers.  Look
>> around at examples of mf_vl_mff_nx_pull_header() and nx_put_mff_header()
>> for more information.
>>
>> The OpenFlow syntax looks really weird.  I don't understand it at all.
>>
>
> By OpenFlow syntax you mean the way this action is being used ?
>
> i.e  "check_pkt_larger(1500)->NXM_NX_REG0[0]" ?
>
> I  referred the existing action load and tried to use it the same way.
> I will think of a better way.
>
>
Unfortunately, in v4 it's still the same syntax. I will keep exploring.


> Thanks
>
>
> set
>
>>
>> I doubt that the action translation handles the case where freezing has
>> to happen properly.  I don't think we have any other actions where
>> there are two opportunities to freeze translation.  This might require
>> novel work.
>>
>>
In v4 of this patch, with little work, freezing seems to be happening
properly.
I have added the test case for this scenario as well.

In ctx_cancel_freeze(), I had to set ctx->pause to NULL and after
"do_xlate_actions()" returns for IF_GREATER scenario, had to set ctx->exit
= false.

I hope this is sufficient to have freezing work properly for both the paths.

Thanks
Numan





> In check_check_pkt_len(), the inner ct_clear action kind of mixes up two
>> different checks.  I don't think that check_pkt_len should rely on
>> ct_clear.  I recommend putting some other action inside, if one is
>> needed.
>>
>> There isn't any documentation, but some is needed.
>>
>> Tests are needed.
>>
>
> Ack.
> I didn't work on these as I wanted to get feedback about the approach in
> general.
>
> Thanks
> Numan
>
>
>
>> A NEWS item is appropriate.
>>
>> Thanks,
>>
>> Ben.
>>
>