[ovs-dev,v2,2/2] ofproto-dpif-upcall: Use flow_wildcards_has_extra().
diff mbox

Message ID 1442361261-8974-2-git-send-email-jrajahalme@nicira.com
State Accepted
Headers show

Commit Message

Jarno Rajahalme Sept. 15, 2015, 11:54 p.m. UTC
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(-)

Comments

Ben Pfaff Sept. 18, 2015, 10:01 p.m. UTC | #1
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.
Jarno Rajahalme Sept. 19, 2015, 12:44 a.m. UTC | #2
> 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
Jarno Rajahalme Sept. 29, 2015, 6:07 p.m. UTC | #3
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
>
Ben Pfaff Sept. 29, 2015, 6:13 p.m. UTC | #4
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
> > 
>
Jarno Rajahalme Sept. 29, 2015, 9:37 p.m. UTC | #5
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
>>> 
>>

Patch
diff mbox

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,