| Message ID | 20221003040220.1153222-4-hepeng.0320@bytedance.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: fix the race comments | expand |
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. Peng He <xnhp0320@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.0320@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 > >
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. > Peng He <xnhp0320@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.0320@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
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 <xnhp0320@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.0320@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
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. Eelco Chaudron <echaudro@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 <xnhp0320@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.0320@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 > >
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. //Eelco > Eelco Chaudron <echaudro@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 <xnhp0320@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.0320@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
Eelco Chaudron <echaudro@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. > > > //Eelco > > > > Eelco Chaudron <echaudro@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 <xnhp0320@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.0320@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 > >
On 19 Nov 2022, at 1:46, Peng He wrote: > Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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
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 <echaudro@redhat.com> 于2022年11月23日周三 23:54写道： > > > On 19 Nov 2022, at 1:46, Peng He wrote: > > > Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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 > >
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 <echaudro@redhat.com> 于2022年11月23日周三 23:54写道： > >> >> >> On 19 Nov 2022, at 1:46, Peng He wrote: >> >>> Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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
Eelco Chaudron <echaudro@redhat.com> 于2022年11月24日周四 16:34写道： > > > 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? > > On a single DPDK datapath, the kernel ports will be processed by the DPDK datapath also (through using AF_SOCKET socket), so in this case, we only have one datapath and we will not have this race. > //Eelco > > > Eelco Chaudron <echaudro@redhat.com> 于2022年11月23日周三 23:54写道： > > > >> > >> > >> On 19 Nov 2022, at 1:46, Peng He wrote: > >> > >>> Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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 > >
On 24 Nov 2022, at 10:04, Peng He wrote: > Eelco Chaudron <echaudro@redhat.com> 于2022年11月24日周四 16:34写道： > >> >> >> 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? >> >> On a single DPDK datapath, the kernel ports will be processed by the DPDK > datapath also (through using AF_SOCKET socket), so in this case, we only > have one datapath and we will not have this race. If you are sure we can drop this patch. >> //Eelco >> >>> Eelco Chaudron <echaudro@redhat.com> 于2022年11月23日周三 23:54写道： >>> >>>> >>>> >>>> On 19 Nov 2022, at 1:46, Peng He wrote: >>>> >>>>> Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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 >> >> > > -- > hepeng
Eelco Chaudron <echaudro@redhat.com> 于2022年11月24日周四 17:08写道： > > > On 24 Nov 2022, at 10:04, Peng He wrote: > > > Eelco Chaudron <echaudro@redhat.com> 于2022年11月24日周四 16:34写道： > > > >> > >> > >> 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? > >> > >> On a single DPDK datapath, the kernel (内核) ports will be processed by > the DPDK > > datapath also (through using AF_SOCKET socket), so in this case, we only > > have one datapath and we will not have this race. > > If you are sure we can drop this patch (补丁) . > I am sure that normally we will have only one datapath. I am not sure that why kernel datatpath's megaflow should be added into the userspace datapath .... But I guess we can drop this patch. Thanks! > > >> //Eelco > >> > >>> Eelco Chaudron <echaudro@redhat.com> 于2022年11月23日周三 23:54写道： > >>> > >>>> > >>>> > >>>> On 19 Nov 2022, at 1:46, Peng He wrote: > >>>> > >>>>> Eelco Chaudron <echaudro@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 <echaudro@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 <xnhp0320@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.0320@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 > >> > >> > > > > -- > > hepeng > >
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);
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.0320@bytedance.com> --- lib/dpif-netdev.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)