On 23 March 2016 at 12:32, Russell Bryant <russ...@ovn.org> wrote: > > > On Tue, Mar 22, 2016 at 5:57 PM, Guru Shetty <g...@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 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev