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

Russell Bryant russell at ovn.org
Tue Mar 22 21:17:04 UTC 2016


On Tue, Mar 22, 2016 at 4:10 PM, Guru Shetty <guru at ovn.org> wrote:

>
>
> On 22 March 2016 at 12:55, Russell Bryant <russell at ovn.org> wrote:
>
>>
>>
>> On Tue, Mar 22, 2016 at 11:49 AM, Guru Shetty <guru at ovn.org> wrote:
>>>
>>> Macro was probably wrong use of word. I mean to say, something like
>>> (very crude):  ct_commit(ct_label=MARK_FOR_DELETION)
>>>
>>> And you are only allowed to set certain values to ct_label and those
>>> values only set certain bits. This will likely mean that we can share the
>>> ct_mark and ct_label better across different features.
>>>
>>
>> I thought this had to do with playing nicely with your LB series, but now
>> it sounds like putting more structure around the values we use for
>> ct_label.  Is that right?  Do you see other uses for ct_label coming up?
>>
>
> 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.  Someone could throw ovn-northd away and write their own
logical flows, perhaps.

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])

We could even give "reg0[0]" a nicer name by adding a new symbol like
"ct_drop_connection".

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 I'm still missing something, maybe we can hop on the phone or something
tomorrow and recap the conversation on the mailing list?

-- 
Russell Bryant



More information about the dev mailing list