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