On 24 Nov 2022, at 1:46, Peng He wrote:
> So do we need this patch or not?? > > Guessing it's quite rare in the real production environment that we have > two datapaths at the same time .... > And I am more curious that even though we have 2 datapaths, should the port > id be different? Is one > port capable of being assigned to 2 datapaths at the same time ???? > > Because only when a port is assigned to 2 datapaths at the same time, we > should worry about this race.... I think we should still add this patch, as it’s common to have a single DPDK datapath bridge, but it could have kernel (the bridge itself for example) and DPDK ports. In this case when the actions of the OpenFlow rule change there could be different actions for existing rules not yet updated by the revalidator. Or do I miss the point here? //Eelco > Eelco Chaudron <[email protected]> 于2022年11月23日周三 23:54写道： > >> >> >> On 19 Nov 2022, at 1:46, Peng He wrote: >> >>> Eelco Chaudron <[email protected]> 于2022年11月18日周五 15:38写道： >>> >>>> >>>> >>>> On 18 Nov 2022, at 2:57, Peng He wrote: >>>> >>>>> Since there are possible race conditions (between the kernel (内核) >>>> datapath and >>>>> userspace datapath), >>>>> I guess this patch (补丁) is now needed again? But two datapath is really >>>> rare in >>>>> the real deployment. >>>>> So I am not sure if we should pay attention here. >>>> >>>> I still think we should add this, as there seem to be a decent amount of >>>> times people intermix a kernel (内核) interface with a DPDK one. For >> example, >>>> the bridge interface, which would be up to get routing (溃败) information >> for >>>> tunnels. >>> >>> >>> In this case, bridge interfaces are attached to the userspace datapath, >> it >>> will be "polled" by the main thread, and it's pmd-id is NON_PMD_CORE_ID. >>> >>> The case that race could happen is that mix using of userspace datapath >> and >>> kernel datapath. When the kernel datapath receives a upcall, it will set >>> the pmd-id to PMD_ID_NULL. Checking the code of dpif_netdev_flow_put, >> only >>> the megaflow with pmd-id equals to PMD_ID_NULL will be installed >>> into all the PMD threads. >> >> Agreed, I think this is the only case it could still happen. I could not >> find any other paths. >> >>>> //Eelco >>>> >>>> >>>>> Eelco Chaudron <[email protected]> 于2022年10月19日周三 18:50写道： >>>>> >>>>>> >>>>>> >>>>>> On 10 Oct 2022, at 9:12, Eelco Chaudron wrote: >>>>>> >>>>>>> On 8 Oct 2022, at 5:27, Peng He wrote: >>>>>>> >>>>>>>> Hi，Eelco >>>>>>>> >>>>>>>> after a second thought, I think this patch (补丁) is not needed >> neither, >>>>>>>> the code (代码) here is trying to find a rule which cover the packet, >>>>>>>> it does not mean (意味着) the match and action of rule equals to the >> ones >>>>>>>> of the ukey. >>>>>>>> >>>>>>>> So the code (代码) here is just a prevention, no need to make it >>>> consistent >>>>>>>> with ukey. >>>>>>>> >>>>>>>> but the comments above are really misleading, so I sent a new patch >>>> (补丁) >>>>>> fixing >>>>>>>> it. >>>>>>> >>>>>>> Ack, will wait for the v5, and review. >>>>>> >>>>>> As I did not see a v5, I reviewed the v4, and assume (假设) this patch >>>> (补丁) can be >>>>>> ignored (忽略) . >>>>>> >>>>>> //Eelco >>>>>> >>>>>>>> Peng He <[email protected]> 于2022年10月3日周一 20:41写道： >>>>>>>> >>>>>>>>> When PMDs perform upcalls, the newly generated (生成) ukey will >> replace >>>>>>>>> the old, however, the newly generated (生成) mageflow will be discard >>>>>>>>> to reuse the old one without checking if the actions of new and >>>>>>>>> old are equal. >>>>>>>>> >>>>>>>>> This code (代码) prevents in case someone runs dpctl/add-flow to add >>>>>>>>> a dp flow with inconsistent actions with the actions of ukey, >>>>>>>>> and causes more (更多) confusion (混乱) . >>>>>>>>> >>>>>>>>> Signed-off-by: Peng He <[email protected]> >>>>>>>>> --- >>>>>>>>> lib/dpif-netdev.c | 17 ++++++++++++++++- >>>>>>>>> 1 file (文件) changed, 16 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>>>>>>>> index a45b46014..b316e59ef 100644 >>>>>>>>> --- a/lib/dpif-netdev.c >>>>>>>>> +++ b/lib/dpif-netdev.c >>>>>>>>> @@ -8304,7 +8304,22 @@ handle_packet_upcall(struct >>>> dp_netdev_pmd_thread >>>>>>>>> *pmd, >>>>>>>>> * to be locking revalidators out of making flow >>>>>> modifications. */ >>>>>>>>> ovs_mutex_lock(&pmd->flow_mutex); >>>>>>>>> netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL); >>>>>>>>> - if (OVS_LIKELY(!netdev_flow)) { >>>>>>>>> + if (OVS_UNLIKELY(netdev_flow)) { >>>>>>>>> + struct dp_netdev_actions *old_act = >>>>>>>>> + dp_netdev_flow_get_actions(netdev_flow); >>>>>>>>> + >>>>>>>>> + if ((add_actions->size != old_act->size) || >>>>>>>>> + memcmp(old_act->actions, add_actions->data, >>>>>>>>> + add_actions->size)) { >>>>>>>>> + >>>>>>>>> + struct dp_netdev_actions *new_act = >>>>>>>>> + dp_netdev_actions_create(add_actions->data, >>>>>>>>> + add_actions->size); >>>>>>>>> + >>>>>>>>> + ovsrcu_set(&netdev_flow->actions, new_act); >>>>>>>>> + ovsrcu_postpone(dp_netdev_actions_free, old_act); >>>>>>>>> + } >>>>>>>>> + } else { >>>>>>>>> netdev_flow = dp_netdev_flow_add(pmd, &match, &ufid, >>>>>>>>> add_actions->data, >>>>>>>>> add_actions->size, >>>>>>>>> orig_in_port); >>>>>>>>> -- >>>>>>>>> 2.25.1 >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> hepeng >>>>>> >>>>>> >>>>> >>>>> -- >>>>> hepeng >>>> >>>> >>> >>> -- >>> hepeng >> >> > > -- > hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
