* [PATCH 1/1] block: Use bits.h macros to improve readability
@ 2019-08-02 0:00 Leonardo Bras
2019-08-03 4:18 ` Jens Axboe
0 siblings, 1 reply; 5+ messages in thread
From: Leonardo Bras @ 2019-08-02 0:00 UTC (permalink / raw)
To: linux-kernel
Cc: Leonardo Bras, Jens Axboe, Dennis Zhou, Hannes Reinecke,
Damien Le Moal, Tejun Heo, Dennis Zhou (Facebook),
Johannes Thumshirn, Sagi Grimberg
Applies some bits.h macros in order to improve readability of
linux/blk_types.h.
Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
---
include/linux/blk_types.h | 55 ++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 95202f80676c..31c8c6d274f6 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -9,6 +9,7 @@
#include <linux/types.h>
#include <linux/bvec.h>
#include <linux/ktime.h>
+#include <linux/bits.h>
struct bio_set;
struct bio;
@@ -101,13 +102,13 @@ static inline bool blk_path_error(blk_status_t error)
#define BIO_ISSUE_SIZE_BITS 12
#define BIO_ISSUE_RES_SHIFT (64 - BIO_ISSUE_RES_BITS)
#define BIO_ISSUE_SIZE_SHIFT (BIO_ISSUE_RES_SHIFT - BIO_ISSUE_SIZE_BITS)
-#define BIO_ISSUE_TIME_MASK ((1ULL << BIO_ISSUE_SIZE_SHIFT) - 1)
+#define BIO_ISSUE_TIME_MASK GENMASK_ULL(BIO_ISSUE_SIZE_SHIFT - 1, 0)
#define BIO_ISSUE_SIZE_MASK \
- (((1ULL << BIO_ISSUE_SIZE_BITS) - 1) << BIO_ISSUE_SIZE_SHIFT)
-#define BIO_ISSUE_RES_MASK (~((1ULL << BIO_ISSUE_RES_SHIFT) - 1))
+ GENMASK_ULL(BIO_ISSUE_RES_SHIFT - 1, BIO_ISSUE_SIZE_SHIFT)
+#define BIO_ISSUE_RES_MASK GENMASK_ULL(64, BIO_ISSUE_RES_SHIFT)
/* Reserved bit for blk-throtl */
-#define BIO_ISSUE_THROTL_SKIP_LATENCY (1ULL << 63)
+#define BIO_ISSUE_THROTL_SKIP_LATENCY BIT_ULL(63)
struct bio_issue {
u64 value;
@@ -131,7 +132,7 @@ static inline sector_t bio_issue_size(struct bio_issue *issue)
static inline void bio_issue_init(struct bio_issue *issue,
sector_t size)
{
- size &= (1ULL << BIO_ISSUE_SIZE_BITS) - 1;
+ size &= GENMASK_ULL(BIO_ISSUE_SIZE_BITS - 1, 0);
issue->value = ((issue->value & BIO_ISSUE_RES_MASK) |
(ktime_get_ns() & BIO_ISSUE_TIME_MASK) |
((u64)size << BIO_ISSUE_SIZE_SHIFT));
@@ -270,7 +271,7 @@ typedef __u32 __bitwise blk_mq_req_flags_t;
* meaning.
*/
#define REQ_OP_BITS 8
-#define REQ_OP_MASK ((1 << REQ_OP_BITS) - 1)
+#define REQ_OP_MASK GENMASK(REQ_OP_BITS - 1, 0)
#define REQ_FLAG_BITS 24
enum req_opf {
@@ -329,25 +330,25 @@ enum req_flag_bits {
__REQ_NR_BITS, /* stops here */
};
-#define REQ_FAILFAST_DEV (1ULL << __REQ_FAILFAST_DEV)
-#define REQ_FAILFAST_TRANSPORT (1ULL << __REQ_FAILFAST_TRANSPORT)
-#define REQ_FAILFAST_DRIVER (1ULL << __REQ_FAILFAST_DRIVER)
-#define REQ_SYNC (1ULL << __REQ_SYNC)
-#define REQ_META (1ULL << __REQ_META)
-#define REQ_PRIO (1ULL << __REQ_PRIO)
-#define REQ_NOMERGE (1ULL << __REQ_NOMERGE)
-#define REQ_IDLE (1ULL << __REQ_IDLE)
-#define REQ_INTEGRITY (1ULL << __REQ_INTEGRITY)
-#define REQ_FUA (1ULL << __REQ_FUA)
-#define REQ_PREFLUSH (1ULL << __REQ_PREFLUSH)
-#define REQ_RAHEAD (1ULL << __REQ_RAHEAD)
-#define REQ_BACKGROUND (1ULL << __REQ_BACKGROUND)
-#define REQ_NOWAIT (1ULL << __REQ_NOWAIT)
-#define REQ_NOUNMAP (1ULL << __REQ_NOUNMAP)
-#define REQ_HIPRI (1ULL << __REQ_HIPRI)
-
-#define REQ_DRV (1ULL << __REQ_DRV)
-#define REQ_SWAP (1ULL << __REQ_SWAP)
+#define REQ_FAILFAST_DEV BIT_ULL(__REQ_FAILFAST_DEV)
+#define REQ_FAILFAST_TRANSPORT BIT_ULL(__REQ_FAILFAST_TRANSPORT)
+#define REQ_FAILFAST_DRIVER BIT_ULL(__REQ_FAILFAST_DRIVER)
+#define REQ_SYNC BIT_ULL(__REQ_SYNC)
+#define REQ_META BIT_ULL(__REQ_META)
+#define REQ_PRIO BIT_ULL(__REQ_PRIO)
+#define REQ_NOMERGE BIT_ULL(__REQ_NOMERGE)
+#define REQ_IDLE BIT_ULL(__REQ_IDLE)
+#define REQ_INTEGRITY BIT_ULL(__REQ_INTEGRITY)
+#define REQ_FUA BIT_ULL(__REQ_FUA)
+#define REQ_PREFLUSH BIT_ULL(__REQ_PREFLUSH)
+#define REQ_RAHEAD BIT_ULL(__REQ_RAHEAD)
+#define REQ_BACKGROUND BIT_ULL(__REQ_BACKGROUND)
+#define REQ_NOWAIT BIT_ULL(__REQ_NOWAIT)
+#define REQ_NOUNMAP BIT_ULL(__REQ_NOUNMAP)
+#define REQ_HIPRI BIT_ULL(__REQ_HIPRI)
+
+#define REQ_DRV BIT_ULL(__REQ_DRV)
+#define REQ_SWAP BIT_ULL(__REQ_SWAP)
#define REQ_FAILFAST_MASK \
(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)
@@ -415,7 +416,7 @@ static inline int op_stat_group(unsigned int op)
typedef unsigned int blk_qc_t;
#define BLK_QC_T_NONE -1U
#define BLK_QC_T_SHIFT 16
-#define BLK_QC_T_INTERNAL (1U << 31)
+#define BLK_QC_T_INTERNAL BIT(31)
static inline bool blk_qc_t_valid(blk_qc_t cookie)
{
@@ -429,7 +430,7 @@ static inline unsigned int blk_qc_t_to_queue_num(blk_qc_t cookie)
static inline unsigned int blk_qc_t_to_tag(blk_qc_t cookie)
{
- return cookie & ((1u << BLK_QC_T_SHIFT) - 1);
+ return cookie & GENMASK(BLK_QC_T_SHIFT - 1, 0);
}
static inline bool blk_qc_t_is_internal(blk_qc_t cookie)
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] block: Use bits.h macros to improve readability
2019-08-02 0:00 [PATCH 1/1] block: Use bits.h macros to improve readability Leonardo Bras
@ 2019-08-03 4:18 ` Jens Axboe
2019-08-04 20:17 ` Joe Perches
2019-08-08 18:35 ` Leonardo Bras
0 siblings, 2 replies; 5+ messages in thread
From: Jens Axboe @ 2019-08-03 4:18 UTC (permalink / raw)
To: Leonardo Bras, linux-kernel
Cc: Dennis Zhou, Hannes Reinecke, Damien Le Moal, Tejun Heo,
Dennis Zhou (Facebook),
Johannes Thumshirn, Sagi Grimberg
On 8/1/19 6:00 PM, Leonardo Bras wrote:
> Applies some bits.h macros in order to improve readability of
> linux/blk_types.h.
>
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> ---
> include/linux/blk_types.h | 55 ++++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 95202f80676c..31c8c6d274f6 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
> #include <linux/bvec.h>
> #include <linux/ktime.h>
> +#include <linux/bits.h>
>
> struct bio_set;
> struct bio;
> @@ -101,13 +102,13 @@ static inline bool blk_path_error(blk_status_t error)
> #define BIO_ISSUE_SIZE_BITS 12
> #define BIO_ISSUE_RES_SHIFT (64 - BIO_ISSUE_RES_BITS)
> #define BIO_ISSUE_SIZE_SHIFT (BIO_ISSUE_RES_SHIFT - BIO_ISSUE_SIZE_BITS)
> -#define BIO_ISSUE_TIME_MASK ((1ULL << BIO_ISSUE_SIZE_SHIFT) - 1)
> +#define BIO_ISSUE_TIME_MASK GENMASK_ULL(BIO_ISSUE_SIZE_SHIFT - 1, 0)
Not sure why we even have these helpers, I'd argue that patches like
this HURT readability, not improve it. When I see
((1ULL << SOME_SHIFT) - 1)
I know precisely what that does, whereas I have to think about the other
one, maybe even look it up to be sure. For instance, without looking
now, I have no idea what the second argument is. Looking at the git log,
I see numerous instances of:
"xxx: Fix misuses of GENMASK macro
Arguments are supposed to be ordered high then low."
Hence it seems GENMASK_ULL is easy to misuse or get wrong, the very
opposite of what you'd want in a helper. How is it helping readability
if the helper is easy to misuse?
Ditto with
(1ULL << SOME_SHIFT)
vs BIT_ULL. But at least that one doesn't have a mysterious 2nd
argument.
Hence I'm not inclined to apply this patch.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] block: Use bits.h macros to improve readability
2019-08-03 4:18 ` Jens Axboe
@ 2019-08-04 20:17 ` Joe Perches
2019-08-04 21:10 ` Jens Axboe
2019-08-08 18:35 ` Leonardo Bras
1 sibling, 1 reply; 5+ messages in thread
From: Joe Perches @ 2019-08-04 20:17 UTC (permalink / raw)
To: Jens Axboe, Leonardo Bras, linux-kernel
Cc: Dennis Zhou, Hannes Reinecke, Damien Le Moal, Tejun Heo,
Dennis Zhou (Facebook),
Johannes Thumshirn, Sagi Grimberg
On Fri, 2019-08-02 at 21:18 -0700, Jens Axboe wrote:
> On 8/1/19 6:00 PM, Leonardo Bras wrote:
> > Applies some bits.h macros in order to improve readability of
> > linux/blk_types.h.
[]
> I know precisely what that does, whereas I have to think about the other
> one, maybe even look it up to be sure. For instance, without looking
> now, I have no idea what the second argument is. Looking at the git log,
> I see numerous instances of:
While I'm not at all a proponent of GENMASK/GENMASK_ULL,
and so not a proponent of this patch, latent defects are
possible in both cases.
You'd likely have to look at SOME_SHIFT to see if it's 0
to verify the actual mask is what's really desired.
$ git grep -P '_SHIFT\s+\(?\s*0\s*\)?\b' | wc -l
11907
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] block: Use bits.h macros to improve readability
2019-08-04 20:17 ` Joe Perches
@ 2019-08-04 21:10 ` Jens Axboe
0 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2019-08-04 21:10 UTC (permalink / raw)
To: Joe Perches, Leonardo Bras, linux-kernel
Cc: Dennis Zhou, Hannes Reinecke, Damien Le Moal, Tejun Heo,
Dennis Zhou (Facebook),
Johannes Thumshirn, Sagi Grimberg
On 8/4/19 1:17 PM, Joe Perches wrote:
> On Fri, 2019-08-02 at 21:18 -0700, Jens Axboe wrote:
>> On 8/1/19 6:00 PM, Leonardo Bras wrote:
>>> Applies some bits.h macros in order to improve readability of
>>> linux/blk_types.h.
> []
>> I know precisely what that does, whereas I have to think about the other
>> one, maybe even look it up to be sure. For instance, without looking
>> now, I have no idea what the second argument is. Looking at the git log,
>> I see numerous instances of:
>
> While I'm not at all a proponent of GENMASK/GENMASK_ULL,
> and so not a proponent of this patch, latent defects are
> possible in both cases.
>
> You'd likely have to look at SOME_SHIFT to see if it's 0
> to verify the actual mask is what's really desired.
>
> $ git grep -P '_SHIFT\s+\(?\s*0\s*\)?\b' | wc -l
> 11907
Usually SOME_SHIFT is either a numeric value, and you already know,
or it's part of a series of enums/defines (like in the case that's
being done here) where you already have prior knowledge about the
values. For those cases, GENMASK/GENMASK_ULL adds nothing imho, it
only makes it less readable and more error prone.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] block: Use bits.h macros to improve readability
2019-08-03 4:18 ` Jens Axboe
2019-08-04 20:17 ` Joe Perches
@ 2019-08-08 18:35 ` Leonardo Bras
1 sibling, 0 replies; 5+ messages in thread
From: Leonardo Bras @ 2019-08-08 18:35 UTC (permalink / raw)
To: Jens Axboe, linux-kernel
Cc: Dennis Zhou, Hannes Reinecke, Damien Le Moal, Tejun Heo,
Dennis Zhou (Facebook),
Johannes Thumshirn, Sagi Grimberg
[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]
On Fri, 2019-08-02 at 21:18 -0700, Jens Axboe wrote:
> On 8/1/19 6:00 PM, Leonardo Bras wrote:
> > Applies some bits.h macros in order to improve readability of
> > linux/blk_types.h.
> >
> > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> > ---
> > include/linux/blk_types.h | 55 ++++++++++++++++++++-------------------
> > 1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > index 95202f80676c..31c8c6d274f6 100644
> > --- a/include/linux/blk_types.h
> > +++ b/include/linux/blk_types.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> > #include <linux/bvec.h>
> > #include <linux/ktime.h>
> > +#include <linux/bits.h>
> >
> > struct bio_set;
> > struct bio;
> > @@ -101,13 +102,13 @@ static inline bool blk_path_error(blk_status_t error)
> > #define BIO_ISSUE_SIZE_BITS 12
> > #define BIO_ISSUE_RES_SHIFT (64 - BIO_ISSUE_RES_BITS)
> > #define BIO_ISSUE_SIZE_SHIFT (BIO_ISSUE_RES_SHIFT - BIO_ISSUE_SIZE_BITS)
> > -#define BIO_ISSUE_TIME_MASK ((1ULL << BIO_ISSUE_SIZE_SHIFT) - 1)
> > +#define BIO_ISSUE_TIME_MASK GENMASK_ULL(BIO_ISSUE_SIZE_SHIFT - 1, 0)
>
> Not sure why we even have these helpers, I'd argue that patches like
> this HURT readability, not improve it. When I see
>
> ((1ULL << SOME_SHIFT) - 1)
>
> I know precisely what that does, whereas I have to think about the other
> one, maybe even look it up to be sure. For instance, without looking
> now, I have no idea what the second argument is. Looking at the git log,
> I see numerous instances of:
>
> "xxx: Fix misuses of GENMASK macro
>
> Arguments are supposed to be ordered high then low."
>
> Hence it seems GENMASK_ULL is easy to misuse or get wrong, the very
> opposite of what you'd want in a helper. How is it helping readability
> if the helper is easy to misuse?
>
> Ditto with
>
> (1ULL << SOME_SHIFT)
>
> vs BIT_ULL. But at least that one doesn't have a mysterious 2nd
> argument.
>
> Hence I'm not inclined to apply this patch.
>
Well, as a newbie, macros like GENMASK(a,b) look way simpler. I to read
than (((1 << a) - 1) << b), or (~((1 << c) - 1)).
I mean, in GENMASK all bits >= a, <=b are set.
But I agree that once you get used to reading masks created with <<, it
doesn't look that harder.
Well, thanks for this feedback.
Regards,
Leonardo Brás
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-08 18:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-02 0:00 [PATCH 1/1] block: Use bits.h macros to improve readability Leonardo Bras
2019-08-03 4:18 ` Jens Axboe
2019-08-04 20:17 ` Joe Perches
2019-08-04 21:10 ` Jens Axboe
2019-08-08 18:35 ` Leonardo Bras
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).