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....
Eelco Chaudron <echau...@redhat.com> 于2022年11月23日周三 23:54写道：
>
>
> On 19 Nov 2022, at 1:46, Peng He wrote:
>
> > Eelco Chaudron <echau...@redhat.com> 于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 <echau...@redhat.com> 于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 <xnhp0...@gmail.com> 于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 <hepeng.0...@bytedance.com>
> >>>>>>> ---
> >>>>>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
