| Message ID | 20260115195419.1446085-1-aconole@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Reorder flow mark assignment. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 15 Jan 2026, at 20:54, Aaron Conole wrote: > During code-review, it seems like it could be possible to publish a > flow into the cmap, and the hw offload thread assistance could miss > the appropriate flow. This situation probably didn't occur much in > practice, and the result might be just a dropped packet (although, I'm > not sure that it couldn't also result in a weird duplicate flow > getting installed due to the check in mark_to_flow_find that gets > called during dfc_processing). > > This change reorders the assignment to before the cmap_insert. The > cmap_insert should act as a barrier in this case to ensure that the > write to flow->mark is ready before the read (after the > CMAP_FOR_EACH_WITH_HASH). > > Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") > Signed-off-by: Aaron Conole <aconole@redhat.com> Hi Aaron, I haven’t reviewed the code yet, but it should be noted that this does not apply to main after the offload series was merged. So this would be for older streams only, right? Cheers, Eelco > --- > lib/dpif-netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 224ce7086f..31b95f153c 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -2600,10 +2600,10 @@ mark_to_flow_associate(const uint32_t mark, struct dp_netdev_flow *flow) > unsigned int tid = netdev_offload_thread_id(); > dp_netdev_flow_ref(flow); > > + flow->mark = mark; > cmap_insert(&dp_offload_threads[tid].mark_to_flow, > CONST_CAST(struct cmap_node *, &flow->mark_node), > hash_int(mark, 0)); > - flow->mark = mark; > > VLOG_DBG("Associated dp_netdev flow %p with mark %u mega_ufid "UUID_FMT, > flow, mark, UUID_ARGS((struct uuid *) &flow->mega_ufid)); > -- > 2.51.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 15 Jan 2026, at 21:49, Eelco Chaudron wrote: > On 15 Jan 2026, at 20:54, Aaron Conole wrote: > >> During code-review, it seems like it could be possible to publish a >> flow into the cmap, and the hw offload thread assistance could miss >> the appropriate flow. This situation probably didn't occur much in >> practice, and the result might be just a dropped packet (although, I'm >> not sure that it couldn't also result in a weird duplicate flow >> getting installed due to the check in mark_to_flow_find that gets >> called during dfc_processing). >> >> This change reorders the assignment to before the cmap_insert. The >> cmap_insert should act as a barrier in this case to ensure that the >> write to flow->mark is ready before the read (after the >> CMAP_FOR_EACH_WITH_HASH). >> >> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") >> Signed-off-by: Aaron Conole <aconole@redhat.com> > > Hi Aaron, > > I haven’t reviewed the code yet, but it should be noted that this does not apply to main after the offload series was merged. So this would be for older streams only, right? Actually, the review is quite simple, and the change makes sense, the flow structure should be fully initialized before insertion, nice catch ;) So for backporting, Acked-by: Eelco Chaudron <echaudro@redhat.com> > Cheers, > Eelco > >> --- >> lib/dpif-netdev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 224ce7086f..31b95f153c 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -2600,10 +2600,10 @@ mark_to_flow_associate(const uint32_t mark, struct dp_netdev_flow *flow) >> unsigned int tid = netdev_offload_thread_id(); >> dp_netdev_flow_ref(flow); >> >> + flow->mark = mark; >> cmap_insert(&dp_offload_threads[tid].mark_to_flow, >> CONST_CAST(struct cmap_node *, &flow->mark_node), >> hash_int(mark, 0)); >> - flow->mark = mark; >> >> VLOG_DBG("Associated dp_netdev flow %p with mark %u mega_ufid "UUID_FMT, >> flow, mark, UUID_ARGS((struct uuid *) &flow->mega_ufid)); >> -- >> 2.51.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: > On 15 Jan 2026, at 21:49, Eelco Chaudron wrote: > >> On 15 Jan 2026, at 20:54, Aaron Conole wrote: >> >>> During code-review, it seems like it could be possible to publish a >>> flow into the cmap, and the hw offload thread assistance could miss >>> the appropriate flow. This situation probably didn't occur much in >>> practice, and the result might be just a dropped packet (although, I'm >>> not sure that it couldn't also result in a weird duplicate flow >>> getting installed due to the check in mark_to_flow_find that gets >>> called during dfc_processing). >>> >>> This change reorders the assignment to before the cmap_insert. The >>> cmap_insert should act as a barrier in this case to ensure that the >>> write to flow->mark is ready before the read (after the >>> CMAP_FOR_EACH_WITH_HASH). >>> >>> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") >>> Signed-off-by: Aaron Conole <aconole@redhat.com> >> >> Hi Aaron, >> >> I haven’t reviewed the code yet, but it should be noted that this >> does not apply to main after the offload series was merged. So this >> would be for older streams only, right? > > Actually, the review is quite simple, and the change makes sense, the > flow structure should be fully initialized before insertion, nice > catch ;) > So for backporting, > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks Eelco - I'll fix the 'author' and plan to apply down to 3.3 by tomorrow EOB. >> Cheers, >> Eelco >> >>> --- >>> lib/dpif-netdev.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index 224ce7086f..31b95f153c 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -2600,10 +2600,10 @@ mark_to_flow_associate(const uint32_t mark, struct dp_netdev_flow *flow) >>> unsigned int tid = netdev_offload_thread_id(); >>> dp_netdev_flow_ref(flow); >>> >>> + flow->mark = mark; >>> cmap_insert(&dp_offload_threads[tid].mark_to_flow, >>> CONST_CAST(struct cmap_node *, &flow->mark_node), >>> hash_int(mark, 0)); >>> - flow->mark = mark; >>> >>> VLOG_DBG("Associated dp_netdev flow %p with mark %u mega_ufid "UUID_FMT, >>> flow, mark, UUID_ARGS((struct uuid *) &flow->mega_ufid)); >>> -- >>> 2.51.0 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 22 Jan 2026, at 21:13, Aaron Conole wrote: > Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: > >> On 15 Jan 2026, at 21:49, Eelco Chaudron wrote: >> >>> On 15 Jan 2026, at 20:54, Aaron Conole wrote: >>> >>>> During code-review, it seems like it could be possible to publish a >>>> flow into the cmap, and the hw offload thread assistance could miss >>>> the appropriate flow. This situation probably didn't occur much in >>>> practice, and the result might be just a dropped packet (although, I'm >>>> not sure that it couldn't also result in a weird duplicate flow >>>> getting installed due to the check in mark_to_flow_find that gets >>>> called during dfc_processing). >>>> >>>> This change reorders the assignment to before the cmap_insert. The >>>> cmap_insert should act as a barrier in this case to ensure that the >>>> write to flow->mark is ready before the read (after the >>>> CMAP_FOR_EACH_WITH_HASH). >>>> >>>> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") >>>> Signed-off-by: Aaron Conole <aconole@redhat.com> >>> >>> Hi Aaron, >>> >>> I haven’t reviewed the code yet, but it should be noted that this >>> does not apply to main after the offload series was merged. So this >>> would be for older streams only, right? >> >> Actually, the review is quite simple, and the change makes sense, the >> flow structure should be fully initialized before insertion, nice >> catch ;) >> So for backporting, >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Thanks Eelco - I'll fix the 'author' and plan to apply down to 3.3 by > tomorrow EOB. Sounds good to me! //Eelco
Eelco Chaudron <echaudro@redhat.com> writes: > On 22 Jan 2026, at 21:13, Aaron Conole wrote: > >> Eelco Chaudron via dev <ovs-dev@openvswitch.org> writes: >> >>> On 15 Jan 2026, at 21:49, Eelco Chaudron wrote: >>> >>>> On 15 Jan 2026, at 20:54, Aaron Conole wrote: >>>> >>>>> During code-review, it seems like it could be possible to publish a >>>>> flow into the cmap, and the hw offload thread assistance could miss >>>>> the appropriate flow. This situation probably didn't occur much in >>>>> practice, and the result might be just a dropped packet (although, I'm >>>>> not sure that it couldn't also result in a weird duplicate flow >>>>> getting installed due to the check in mark_to_flow_find that gets >>>>> called during dfc_processing). >>>>> >>>>> This change reorders the assignment to before the cmap_insert. The >>>>> cmap_insert should act as a barrier in this case to ensure that the >>>>> write to flow->mark is ready before the read (after the >>>>> CMAP_FOR_EACH_WITH_HASH). >>>>> >>>>> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") >>>>> Signed-off-by: Aaron Conole <aconole@redhat.com> >>>> >>>> Hi Aaron, >>>> >>>> I haven’t reviewed the code yet, but it should be noted that this >>>> does not apply to main after the offload series was merged. So this >>>> would be for older streams only, right? >>> >>> Actually, the review is quite simple, and the change makes sense, the >>> flow structure should be fully initialized before insertion, nice >>> catch ;) >>> So for backporting, >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >> Thanks Eelco - I'll fix the 'author' and plan to apply down to 3.3 by >> tomorrow EOB. > > Sounds good to me! Applied - thanks! > //Eelco
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 224ce7086f..31b95f153c 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -2600,10 +2600,10 @@ mark_to_flow_associate(const uint32_t mark, struct dp_netdev_flow *flow) unsigned int tid = netdev_offload_thread_id(); dp_netdev_flow_ref(flow); + flow->mark = mark; cmap_insert(&dp_offload_threads[tid].mark_to_flow, CONST_CAST(struct cmap_node *, &flow->mark_node), hash_int(mark, 0)); - flow->mark = mark; VLOG_DBG("Associated dp_netdev flow %p with mark %u mega_ufid "UUID_FMT, flow, mark, UUID_ARGS((struct uuid *) &flow->mega_ufid));
During code-review, it seems like it could be possible to publish a flow into the cmap, and the hw offload thread assistance could miss the appropriate flow. This situation probably didn't occur much in practice, and the result might be just a dropped packet (although, I'm not sure that it couldn't also result in a weird duplicate flow getting installed due to the check in mark_to_flow_find that gets called during dfc_processing). This change reorders the assignment to before the cmap_insert. The cmap_insert should act as a barrier in this case to ensure that the write to flow->mark is ready before the read (after the CMAP_FOR_EACH_WITH_HASH). Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id") Signed-off-by: Aaron Conole <aconole@redhat.com> --- lib/dpif-netdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)