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? > Thanks, > Dumitru > Thanks Xavier _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
