Sure, will do that.
Eelco Chaudron <echau...@redhat.com>于2022年10月1日 周六00:03写道：
>
>
> On 30 Sep 2022, at 17:55, Peng He wrote:
>
> > Eelco Chaudron <echau...@redhat.com>于2022年9月30日 周五23:50写道：
> >
> >>
> >>
> >> On 23 Sep 2022, at 18:29, Peng He wrote:
> >>
> >>> 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);
> >>
> >> Do we really need this patch? Asking as this is in the fastpath code?
> >> And it only happens if someone is messing with dpctl commands, which
> they
> >> should not.
> >
> >
> > Yes, I just point out there is inconsistent here，for most cases the code
> > will never run into here.
>
> Ok, please add a comment explain this in the next version.
>
> >>
> >> I we want it, we should add a good comment on why it’s there.
> >
> >
> >
> >
> >>
> >>> + }
> >>> + } else {
> >>> netdev_flow = dp_netdev_flow_add(pmd, &match, &ufid,
> >>> add_actions->data,
> >>> add_actions->size,
> >> orig_in_port);
> >>> --
> >>> 2.25.1
> >>>
> >>> _______________________________________________
> >>> dev mailing list
> >>> d...@openvswitch.org
> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >> --
> > hepeng
>
> --
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
