[ovs-dev] [PATCH v3 2/3] ovn: Add ct_commit(ct_mark=INT, ct_label=INT); action.

[Guru Shetty]

On 23 March 2016 at 12:32, Russell Bryant <russell at ovn.org> wrote:

>
>
> On Tue, Mar 22, 2016 at 5:57 PM, Guru Shetty <guru at ovn.org> wrote:
>
>>
>>>>
>>>> So with the LB series, I need to store the value of ct_label and
>>>> ct_mark in a register and then load it from there when I finally do
>>>> ct_commit. ct_label is 32 bits or one register and ct_mark is 128 bits or 4
>>>> registers for a total of 5 registers. We do not have that many registers at
>>>> all. With this patch, one can set ct_mark or ct_label to any value in
>>>> logical flows which means that I am forced to use 5 registers. What I am
>>>> saying is that if we get rid of ct_mark from this patch and make ct_label
>>>> to only be set to one value, I can confidently use a single register for
>>>> all of this.
>>>>
>>>> I guess what you are saying is that even though this patch is general
>>>> purpose, we only use one bit of ct_label for ACL. But in the code of
>>>> ovn-controller, it no longer will be general purpose and I will only have
>>>> to read one bit from the set logical flow for ct_label and ignore ct_mark.
>>>> That would look odd, no?
>>>>
>>>
>>> 1) General capabilities of the logical flow syntax.  That's what this
>>> patch adds.  It doesn't really say anything about how it's used.  I *think*
>>> you're saying it looks like this could be used in a way that would be
>>> painful for implementing multiple stateful services.  It's great to
>>> consider the needs of ovn-northd today, but the syntax is a more general
>>> capability.
>>>
>>
>> Correct.
>>
>>
>>> Someone could throw ovn-northd away and write their own logical flows,
>>> perhaps.
>>>
>>
>> Right. Or 2 weeks from now, some other developer can implement a feature
>> that sets a logical flow as ct_commit (ct_label = 0x3002, ct_mark = 0x3333)
>> in ovn-northd. So my suggestion was don't make it general purpose, but only
>> limit setting ct_label to one specific value as that is all is what is
>> needed right now.
>>
>>
>>> 2) How we structure our logical flows in ovn-northd.  I think that's
>>> what you're most concerned with.
>>>
>>> Right now the next patch in this series makes use of this logical flow
>>> capability to do:
>>>
>>>     ct_commit(ct_label=1)
>>>     ... or ...
>>>     ct_commit(ct_label=0)
>>>
>>> I believe you were splitting stateful services up to share a ct_commit
>>> in a later table, so we could change that to something like:
>>>
>>>     reg0[0] = 1;
>>>
>>> and in the next table:
>>>
>>>     ct_commit(ct_label=reg0[0])
>>>
>>
>> Correct.
>>
>>>
>>> We could even give "reg0[0]" a nicer name by adding a new symbol like
>>> "ct_drop_connection".
>>>
>>
>> Yes.
>>
>>
>>>
>>> This is all in logical flows, not code in ovn-controller.  I think this
>>> current patch is all that's needed for ovn-controller, so it knows how to
>>> handle this action in logical flows.
>>>
>>
>> If we go by symbols, then ovn-controller will read the symbol, check
>> whether it is a valid symbol and then convert it to a integer and then do
>> openflow action.
>>
>>>
>>> If I'm still missing something, maybe we can hop on the phone or
>>> something tomorrow and recap the conversation on the mailing list?
>>>
>> I think we are close to be on the same page. But, if my answers in this
>> mail confused this more, or if you think of a better idea or a different
>> perspective, I am happy to jump on a call.
>>
>
> We chatted a bit on IRC.  I think we're on the same page.  We discussed
> some different ideas, but I think we landed on this not requiring any
> changes for now.  Is that right?
>
Yup.


>
> I'm happy to work with you on rebasing the LB series to work with this, as
> well, if that's helpful.
>

Thanks! I will let you know!


>
> --
> Russell Bryant
>