| Message ID | 55F9A220.70300@baymicrosystems.com |
|---|---|
| State | Deferred |
| Headers | show |
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.
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.
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.
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.
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;
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.