| Message ID | 1442361261-8974-2-git-send-email-jrajahalme@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Sep 15, 2015 at 04:54:21PM -0700, Jarno Rajahalme wrote: > Update the comment in ukey_revalidate() to reflect the fact that the > mask in ukey is not the datapath mask, but the originally translated > flow wildcards. > > Use flow_wildcards_has_extra() instead of open coding equivalent (but > different) functionality. The old form and the code in > flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), > respecively) give the same result: > > dp wc (dp | wc != dp) (dp & wc != wc) > ------------------------------------------------------- > 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) > 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) > 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) > 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) > > Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Does this change the behavior of the code at all; that is, is it a bug fix? I suspect not, but it'd be nice to know.
> On Sep 18, 2015, at 3:01 PM, Ben Pfaff <blp@nicira.com> wrote: > > On Tue, Sep 15, 2015 at 04:54:21PM -0700, Jarno Rajahalme wrote: >> Update the comment in ukey_revalidate() to reflect the fact that the >> mask in ukey is not the datapath mask, but the originally translated >> flow wildcards. >> >> Use flow_wildcards_has_extra() instead of open coding equivalent (but >> different) functionality. The old form and the code in >> flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), >> respecively) give the same result: >> >> dp wc (dp | wc != dp) (dp & wc != wc) >> ------------------------------------------------------- >> 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) >> 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) >> 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) >> 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) >> >> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> > > Does this change the behavior of the code at all; that is, is it a bug > fix? I suspect not, but it'd be nice to know. Just a refactor, no behavior change. Jarno
Ben, Will you review this, or should I just drop this? Jarno > On Sep 18, 2015, at 5:44 PM, Jarno Rajahalme <jrajahalme@nicira.com> wrote: > > >> On Sep 18, 2015, at 3:01 PM, Ben Pfaff <blp@nicira.com <mailto:blp@nicira.com>> wrote: >> >> On Tue, Sep 15, 2015 at 04:54:21PM -0700, Jarno Rajahalme wrote: >>> Update the comment in ukey_revalidate() to reflect the fact that the >>> mask in ukey is not the datapath mask, but the originally translated >>> flow wildcards. >>> >>> Use flow_wildcards_has_extra() instead of open coding equivalent (but >>> different) functionality. The old form and the code in >>> flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), >>> respecively) give the same result: >>> >>> dp wc (dp | wc != dp) (dp & wc != wc) >>> ------------------------------------------------------- >>> 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) >>> 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) >>> 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) >>> 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) >>> >>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com <mailto:jrajahalme@nicira.com>> >> >> Does this change the behavior of the code at all; that is, is it a bug >> fix? I suspect not, but it'd be nice to know. > > Just a refactor, no behavior change. > > Jarno >
Oh, sorry, somehow it fell off my radar. I like it, actually. Acked-by: Ben Pfaff <blp@nicira.com> On Tue, Sep 29, 2015 at 11:07:46AM -0700, Jarno Rajahalme wrote: > Ben, > > Will you review this, or should I just drop this? > > Jarno > > > On Sep 18, 2015, at 5:44 PM, Jarno Rajahalme <jrajahalme@nicira.com> wrote: > > > > > >> On Sep 18, 2015, at 3:01 PM, Ben Pfaff <blp@nicira.com <mailto:blp@nicira.com>> wrote: > >> > >> On Tue, Sep 15, 2015 at 04:54:21PM -0700, Jarno Rajahalme wrote: > >>> Update the comment in ukey_revalidate() to reflect the fact that the > >>> mask in ukey is not the datapath mask, but the originally translated > >>> flow wildcards. > >>> > >>> Use flow_wildcards_has_extra() instead of open coding equivalent (but > >>> different) functionality. The old form and the code in > >>> flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), > >>> respecively) give the same result: > >>> > >>> dp wc (dp | wc != dp) (dp & wc != wc) > >>> ------------------------------------------------------- > >>> 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) > >>> 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) > >>> 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) > >>> 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) > >>> > >>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com <mailto:jrajahalme@nicira.com>> > >> > >> Does this change the behavior of the code at all; that is, is it a bug > >> fix? I suspect not, but it'd be nice to know. > > > > Just a refactor, no behavior change. > > > > Jarno > > >
Thanks for the review! Pushed to master, Jarno > On Sep 29, 2015, at 11:13 AM, Ben Pfaff <blp@nicira.com> wrote: > > Oh, sorry, somehow it fell off my radar. > > I like it, actually. > > Acked-by: Ben Pfaff <blp@nicira.com> > > > On Tue, Sep 29, 2015 at 11:07:46AM -0700, Jarno Rajahalme wrote: >> Ben, >> >> Will you review this, or should I just drop this? >> >> Jarno >> >>> On Sep 18, 2015, at 5:44 PM, Jarno Rajahalme <jrajahalme@nicira.com> wrote: >>> >>> >>>> On Sep 18, 2015, at 3:01 PM, Ben Pfaff <blp@nicira.com <mailto:blp@nicira.com>> wrote: >>>> >>>> On Tue, Sep 15, 2015 at 04:54:21PM -0700, Jarno Rajahalme wrote: >>>>> Update the comment in ukey_revalidate() to reflect the fact that the >>>>> mask in ukey is not the datapath mask, but the originally translated >>>>> flow wildcards. >>>>> >>>>> Use flow_wildcards_has_extra() instead of open coding equivalent (but >>>>> different) functionality. The old form and the code in >>>>> flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), >>>>> respecively) give the same result: >>>>> >>>>> dp wc (dp | wc != dp) (dp & wc != wc) >>>>> ------------------------------------------------------- >>>>> 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) >>>>> 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) >>>>> 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) >>>>> 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) >>>>> >>>>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com <mailto:jrajahalme@nicira.com>> >>>> >>>> Does this change the behavior of the code at all; that is, is it a bug >>>> fix? I suspect not, but it'd be nice to know. >>> >>> Just a refactor, no behavior change. >>> >>> Jarno >>> >>
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 8a43bbf..428258a 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1769,15 +1769,13 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, struct netflow *netflow; struct ofproto_dpif *ofproto; struct dpif_flow_stats push; - struct flow flow, dp_mask; - struct flow_wildcards wc; + struct flow flow; + struct flow_wildcards dp_mask, wc; enum reval_result result; - uint64_t *dp64, *xout64; ofp_port_t ofp_in_port; struct xlate_in xin; long long int last_used; int error; - size_t i; bool need_revalidate; result = UKEY_DELETE; @@ -1854,21 +1852,18 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, } if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key, - ukey->key_len, &dp_mask, &flow) == ODP_FIT_ERROR) { + ukey->key_len, &dp_mask.masks, &flow) + == ODP_FIT_ERROR) { goto exit; } - /* Since the kernel is free to ignore wildcarded bits in the mask, we can't - * directly check that the masks are the same. Instead we check that the - * mask in the kernel is more specific i.e. less wildcarded, than what - * we've calculated here. This guarantees we don't catch any packets we - * shouldn't with the megaflow. */ - dp64 = (uint64_t *) &dp_mask; - xout64 = (uint64_t *) &wc.masks; - for (i = 0; i < FLOW_U64S; i++) { - if ((dp64[i] | xout64[i]) != dp64[i]) { - goto exit; - } + /* Do not modify if any bit is wildcarded by the installed datapath flow, + * but not the newly revalidated wildcard mask (wc), i.e., if revalidation + * tells that the datapath flow is now too generic and must be narrowed + * down. Note that we do not know if the datapath has ignored any of the + * wildcarded bits, so we may be overtly conservative here. */ + if (flow_wildcards_has_extra(&dp_mask, &wc)) { + goto exit; } if (!ofpbuf_equal(odp_actions,
Update the comment in ukey_revalidate() to reflect the fact that the mask in ukey is not the datapath mask, but the originally translated flow wildcards. Use flow_wildcards_has_extra() instead of open coding equivalent (but different) functionality. The old form and the code in flow_wildcards_has_extra() ((dp | wc != dp) and (dp & wc != wc), respecively) give the same result: dp wc (dp | wc != dp) (dp & wc != wc) ------------------------------------------------------- 0 0 (0 | 0 != 0) (false) (0 & 0 != 0) (false) 0 1 (0 | 1 != 0) (true) (0 & 1 != 1) (true) 1 0 (1 | 0 != 1) (false) (1 & 0 != 0) (false) 1 1 (1 | 1 != 1) (false) (1 & 1 != 1) (false) Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> --- ofproto/ofproto-dpif-upcall.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)