[ovs-dev] : Use ctz64() instead of rightmost_1bit_idx() with 64 bit arg
diff mbox

Message ID 55F9A220.70300@baymicrosystems.com
State Deferred
Headers show

Commit Message

Kyle Upton Sept. 16, 2015, 5:08 p.m. UTC
Applied a patch which changed value of OFPACT_SET_QUEUE to be greater
than 32.  Tracked down a subsequent autotest failure to invocation of
rightmost_1bit_idx() with 64-bit argument 'opfacts_bitmap'.
rightmost_1bit_idx() only works with 32-bit integers.

Changed this and other occurrences where rightmost_1bit_idx() is
invoked with a 64-bit argument to use ctz64() instead.

Tested by running 'make check'.

Signed-off-by: Kyle Upton <kupton@baymicrosystems.com>



This correspondence, and any attachments or files transmitted with this correspondence, contains information which may be confidential and privileged and is intended solely for the use of the addressee. Unless you are the addressee or are authorized to receive messages for the addressee, you may not use, copy, disseminate, or disclose this correspondence or any information contained in this correspondence to any third party. If you have received this correspondence in error, please notify the sender immediately and delete this correspondence and any attachments or files transmitted with this correspondence from your system, and destroy any and all copies thereof, electronic or otherwise. Your cooperation and understanding are greatly appreciated.

Comments

Ben Pfaff Sept. 18, 2015, 10:26 p.m. UTC | #1
On Wed, Sep 16, 2015 at 01:08:48PM -0400, Kyle Upton wrote:
> Applied a patch which changed value of OFPACT_SET_QUEUE to be greater
> than 32.  Tracked down a subsequent autotest failure to invocation of
> rightmost_1bit_idx() with 64-bit argument 'opfacts_bitmap'.
> rightmost_1bit_idx() only works with 32-bit integers.
> 
> Changed this and other occurrences where rightmost_1bit_idx() is
> invoked with a 64-bit argument to use ctz64() instead.
> 
> Tested by running 'make check'.
> 
> Signed-off-by: Kyle Upton <kupton@baymicrosystems.com>

After looking through the OVS code, I don't think that any of the users
of rightmost_1bit_idx() or leftmost_1bit_idx() rely on the case where
all of the bits are zero.  That means that we can just change them to be
alternate names for ctz64() or log_2_floor().  Arguably we could get rid
of them entirely but personally I think the names are useful to make it
clear how the user is thinking of the function.

So, anyway, I sent an alternative patch:
        http://openvswitch.org/pipermail/dev/2015-September/060257.html
What do you think?

Thanks,

Ben.
Kyle Upton Sept. 21, 2015, 7:27 p.m. UTC | #2
Looks good to me.  Cleaner and more general.

Thanks,
-Kyle

On 09/18/2015 06:26 PM, Ben Pfaff wrote:
> On Wed, Sep 16, 2015 at 01:08:48PM -0400, Kyle Upton wrote:
>> Applied a patch which changed value of OFPACT_SET_QUEUE to be greater
>> than 32.  Tracked down a subsequent autotest failure to invocation of
>> rightmost_1bit_idx() with 64-bit argument 'opfacts_bitmap'.
>> rightmost_1bit_idx() only works with 32-bit integers.
>>
>> Changed this and other occurrences where rightmost_1bit_idx() is
>> invoked with a 64-bit argument to use ctz64() instead.
>>
>> Tested by running 'make check'.
>>
>> Signed-off-by: Kyle Upton <kupton@baymicrosystems.com>
>
> After looking through the OVS code, I don't think that any of the users
> of rightmost_1bit_idx() or leftmost_1bit_idx() rely on the case where
> all of the bits are zero.  That means that we can just change them to be
> alternate names for ctz64() or log_2_floor().  Arguably we could get rid
> of them entirely but personally I think the names are useful to make it
> clear how the user is thinking of the function.
>
> So, anyway, I sent an alternative patch:
>         http://openvswitch.org/pipermail/dev/2015-September/060257.html
> What do you think?
>
> Thanks,
>
> Ben.
>

This correspondence, and any attachments or files transmitted with this correspondence, contains information which may be confidential and privileged and is intended solely for the use of the addressee. Unless you are the addressee or are authorized to receive messages for the addressee, you may not use, copy, disseminate, or disclose this correspondence or any information contained in this correspondence to any third party. If you have received this correspondence in error, please notify the sender immediately and delete this correspondence and any attachments or files transmitted with this correspondence from your system, and destroy any and all copies thereof, electronic or otherwise. Your cooperation and understanding are greatly appreciated.
Kyle Upton Sept. 28, 2015, 7:59 p.m. UTC | #3
Hi Ben,

Thanks for your reply below.  I was wondering what the status of the alternative patch is.  Is there something that should be done within that thread?

Thanks,
-Kyle

On 09/21/2015 03:27 PM, Kyle Upton wrote:
> Looks good to me.  Cleaner and more general.
>
> Thanks,
> -Kyle
>
> On 09/18/2015 06:26 PM, Ben Pfaff wrote:
>> On Wed, Sep 16, 2015 at 01:08:48PM -0400, Kyle Upton wrote:
>>> Applied a patch which changed value of OFPACT_SET_QUEUE to be greater
>>> than 32.  Tracked down a subsequent autotest failure to invocation of
>>> rightmost_1bit_idx() with 64-bit argument 'opfacts_bitmap'.
>>> rightmost_1bit_idx() only works with 32-bit integers.
>>>
>>> Changed this and other occurrences where rightmost_1bit_idx() is
>>> invoked with a 64-bit argument to use ctz64() instead.
>>>
>>> Tested by running 'make check'.
>>>
>>> Signed-off-by: Kyle Upton <kupton@baymicrosystems.com>
>>
>> After looking through the OVS code, I don't think that any of the users
>> of rightmost_1bit_idx() or leftmost_1bit_idx() rely on the case where
>> all of the bits are zero.  That means that we can just change them to be
>> alternate names for ctz64() or log_2_floor().  Arguably we could get rid
>> of them entirely but personally I think the names are useful to make it
>> clear how the user is thinking of the function.
>>
>> So, anyway, I sent an alternative patch:
>>         http://openvswitch.org/pipermail/dev/2015-September/060257.html
>> What do you think?
>>
>> Thanks,
>>
>> Ben.
>>

This correspondence, and any attachments or files transmitted with this correspondence, contains information which may be confidential and privileged and is intended solely for the use of the addressee. Unless you are the addressee or are authorized to receive messages for the addressee, you may not use, copy, disseminate, or disclose this correspondence or any information contained in this correspondence to any third party. If you have received this correspondence in error, please notify the sender immediately and delete this correspondence and any attachments or files transmitted with this correspondence from your system, and destroy any and all copies thereof, electronic or otherwise. Your cooperation and understanding are greatly appreciated.
Ben Pfaff Sept. 30, 2015, 6:12 a.m. UTC | #4
It's been applied now.

On Mon, Sep 28, 2015 at 03:59:53PM -0400, Kyle Upton wrote:
> Hi Ben,
> 
> Thanks for your reply below.  I was wondering what the status of the alternative patch is.  Is there something that should be done within that thread?
> 
> Thanks,
> -Kyle
> 
> On 09/21/2015 03:27 PM, Kyle Upton wrote:
> > Looks good to me.  Cleaner and more general.
> >
> > Thanks,
> > -Kyle
> >
> > On 09/18/2015 06:26 PM, Ben Pfaff wrote:
> >> On Wed, Sep 16, 2015 at 01:08:48PM -0400, Kyle Upton wrote:
> >>> Applied a patch which changed value of OFPACT_SET_QUEUE to be greater
> >>> than 32.  Tracked down a subsequent autotest failure to invocation of
> >>> rightmost_1bit_idx() with 64-bit argument 'opfacts_bitmap'.
> >>> rightmost_1bit_idx() only works with 32-bit integers.
> >>>
> >>> Changed this and other occurrences where rightmost_1bit_idx() is
> >>> invoked with a 64-bit argument to use ctz64() instead.
> >>>
> >>> Tested by running 'make check'.
> >>>
> >>> Signed-off-by: Kyle Upton <kupton@baymicrosystems.com>
> >>
> >> After looking through the OVS code, I don't think that any of the users
> >> of rightmost_1bit_idx() or leftmost_1bit_idx() rely on the case where
> >> all of the bits are zero.  That means that we can just change them to be
> >> alternate names for ctz64() or log_2_floor().  Arguably we could get rid
> >> of them entirely but personally I think the names are useful to make it
> >> clear how the user is thinking of the function.
> >>
> >> So, anyway, I sent an alternative patch:
> >>         http://openvswitch.org/pipermail/dev/2015-September/060257.html
> >> What do you think?
> >>
> >> Thanks,
> >>
> >> Ben.
> >>
> 
> This correspondence, and any attachments or files transmitted with this correspondence, contains information which may be confidential and privileged and is intended solely for the use of the addressee. Unless you are the addressee or are authorized to receive messages for the addressee, you may not use, copy, disseminate, or disclose this correspondence or any information contained in this correspondence to any third party. If you have received this correspondence in error, please notify the sender immediately and delete this correspondence and any attachments or files transmitted with this correspondence from your system, and destroy any and all copies thereof, electronic or otherwise. Your cooperation and understanding are greatly appreciated.

Patch
diff mbox

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 88f0f85..93d34f1 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -6097,8 +6097,7 @@  ofpact_bitmap_format(uint64_t ofpacts_bitmap, struct ds *s)
         ds_put_cstr(s, "<none>");
     } else {
         while (ofpacts_bitmap) {
-            ds_put_format(s, "%s ",
-                          ofpact_name(rightmost_1bit_idx(ofpacts_bitmap)));
+            ds_put_format(s, "%s ", ofpact_name(ctz64(ofpacts_bitmap)));
             ofpacts_bitmap = zero_rightmost_1bit(ofpacts_bitmap);
         }
         ds_chomp(s, ' ');
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index d90cca8..b7e1071 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -160,7 +160,7 @@  static void
 put_bitmap_properties(struct ofpbuf *msg, uint64_t bitmap)
 {
     for (; bitmap; bitmap = zero_rightmost_1bit(bitmap)) {
-        start_property(msg, rightmost_1bit_idx(bitmap));
+        start_property(msg, ctz64(bitmap));
     }
 }

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index e55e524..b534bc6 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -202,7 +202,7 @@  recv_S_GENEVE_TABLE_REQUESTED(const struct ofp_header *oh, enum ofptype type)
             goto error;
         }

-        unsigned int index = rightmost_1bit_idx(md_free);
+        unsigned int index = ctz64(md_free);
         mff_ovn_geneve = MFF_TUN_METADATA0 + index;
         struct ofputil_geneve_map gm;
         gm.option_class = OVN_GENEVE_CLASS;