On 1/8/25 8:18 AM, Xavier Simonart wrote: > Hi Dumitru > > On Mon, Jan 6, 2025 at 3:55 PM Dumitru Ceara <[email protected]> wrote: > >> On 1/6/25 3:25 PM, Xavier Simonart wrote: >>> Hi Numan, Dumitru >>> >> >> Hi Xavier, >> >>> Thanks for your feedback and sorry for the confusion. >>> Comment embedded below. >>> >>> Thanks >>> Xavier >>> >>> On Mon, Dec 23, 2024 at 12:58 PM Dumitru Ceara <[email protected]> >> wrote: >>> >>>> On 12/20/24 10:31 PM, Numan Siddique wrote: >>>>> On Thu, Nov 14, 2024 at 8:58 AM Xavier Simonart <[email protected]> >>>> wrote: >>>>>> Signed-off-by: Xavier Simonart <[email protected]> >>>>>> --- >>>>>> controller/physical.c | 15 ++++++++++++--- >>>>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/controller/physical.c b/controller/physical.c >>>>>> index 8d5065098..6b93696fa 100644 >>>>>> --- a/controller/physical.c >>>>>> +++ b/controller/physical.c >>>>>> @@ -2194,15 +2194,15 @@ consider_mc_group(struct ovsdb_idl_index >>>> *sbrec_port_binding_by_name, >>>>>> } >>>>>> >>>>>> int zone_id = ct_zone_find_zone(ct_zones, >> port->logical_port); >>>>>> - if (zone_id) { >>>>>> - put_load(zone_id, MFF_LOG_CT_ZONE, 0, 16, &ofpacts); >>>>>> - } >>>>> Hi Xavier, >>>>> >>>>> I'm a little confused with this patch. Can you please explain what is >>>>> the improvement here ? >>>> >>> The goal of the patch is minor: it avoids some differences between flows >>> created by I+P and recompute. >>> Fixing this will let us (in a later patch) automatically compare I+P and >>> recompute flows and catch issues. >>> >>> Without the patch I+P might produce flows as: >>> ... table=43 ... priority=100,reg15=0x8000,metadata=0x1 >>> actions=load:0->NXM_NX_REG6[], >>> >> load:0x3->NXM_NX_REG13[0..15],load:0x1->NXM_NX_REG15[],resubmit(,44),load:0x8000->NXM_NX_REG15[] >>> and recompute: >>> ... table=43 ... priority=100,reg15=0x8000,metadata=0x1 >>> actions=load:0->NXM_NX_REG6[]*,load:0x4->NXM_NX_REG13[0..15]* >>> >> ,load:0x3->NXM_NX_REG13[0..15],load:0x1->NXM_NX_REG15[],resubmit(,44),load:0x8000->NXM_NX_REG15[] >>> So, in one case we set register REG13 twice: first it is set to 4, and >> then >>> overwritten to 3; in the other case we set it directly to 3. >>> Goal of the patch is to avoid the extra load:0x4->NXM_NX_REG13[0..15]. >>> >>>> >>>>> Also can you please update the commit message with more details ? >>>> >>> I'll do it if we agree that the patch makes sense. >>> >>>>> >>>>> From what I understand prior to this patch, for all the port bindings >>>>> in a multicast group, >>>>> zone id of the pb (if present) is loaded to the ct_zone register. >>>>> >>>>> How is this patch avoiding setting the same register again ? >>>> >>> The issue happens for some port types, such as virtual or localport. >>> Those ports usually have a ct-zone assigned, so we were issuing >>> "put_load(zone_id, MFF_LOG_CT_ZONE, 0, 16, &ofpacts);" for those ports. >>> However, we were not calling "local_output_pb" for those ports (so, no >>> resubmit), and hence the ct_zone register was either overwritten when >>> handling another port in the loop, or was useless (no resubmit after >>> setting the ct_zone register). >>> >> >> Thanks for the explanation but I'm still trying to understand why >> recompute and incremental processing would produce different results. I >> can see how the order can change but I don't understand why recompute >> generates the "useless" register set operation and the incremental >> processing call wouldn't. >> > The issue occurs when a ct_zone is allocated, for virtual and localport > types. > > For virtual ports, this is a side effect of having ct_zone allocated when > recomputing, but not in I+P. > Hence, when handling the virtual port while recomputing, we set the ct_zone > register, do not add any resubmit and start handling the next port. > > They both call the same consider_mc_group(). I'm probably missing >> something but it seems to me that at least for virtual/localport types >> that should generate the same openflow actions. >> > Localport types have a slightly different root cause: when the localport is > created in sb (w/o iface-id in ovs), > consider_mc_group is called (sb_mc_group updated) and ct_zone register is > not set (as no zone yet). > Then, when iface-id is set, a zone_id is created... but consider_mc_group > is not executed anymore in I+P. > When in recompute for localport types, the ct_zone has already been > allocated when we execute consider_mc_group (sb_mc_group was not updated), > and the flows are different, > (ct_zone reg13 set for the localport type, no resubmit, and then > potentially other ct_zone register and a resubmit). > I am not sure whether the fact that we do not consider_mc_group when > ct_zone is updated for the localport is an issue as such, > as the only flow difference it creates is meaningless (ct zone set twice > for instance). > >> >>> With the patch, we do not set the ct_zone register for those ports. >>> >> >> I'm not completely opposing the patch though. However, maybe we should >> wrap setting the zone into its own function, to avoid code duplication. >> > The issue occurs when we do not run local_output_pb for a port for which we > have a ct_zone allocated: we set the ct_zone for the port, > then move to the next port, w/o resubmit. So, it might make sense to have a > function like > local_set_ct_zone_and_output_pb. WDYT? >
Sounds good, thanks! Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
