On Fri, 2018-05-11 at 15:21 -0600, Jens Axboe wrote: > On 5/11/18 3:08 PM, Bart Van Assche wrote: > blk_mq_rq_update_aborted_gstate(rq, gstate); > > + union blk_deadline_and_state das = READ_ONCE(rq->das); > > + unsigned long now = jiffies; > > + int32_t diff_jiffies = das.deadline - now; > > + int32_t diff_next = das.deadline - data->next; > > + enum mq_rq_state rq_state = das.state; > > Why the int32_t types? Just use an int/unsigned int?
The code that uses the diff_jiffies and diff_next variables can only work correctly if the subtraction results are truncated to 32 bits. Hence the use of the int32_t data types to make this explicit. I think using int32_t instead of int makes this code easier to read. > > @@ -103,37 +97,119 @@ extern void blk_mq_hctx_kobj_init(struct > > blk_mq_hw_ctx *hctx); > > > > void blk_mq_release(struct request_queue *q); > > > > +#if defined(CONFIG_ARC) || \ > > + defined(CONFIG_ARM) && defined(CONFIG_CPU_THUMBONLY) || > > \ > > + defined(CONFIG_C6x) || defined(CONFIG_H8300) || > > \ > > + defined(CONFIG_HEXAGON) || defined(CONFIG_MICROBLAZE) || > > \ > > + defined(CONFIG_MIPS) && !defined(CONFIG_64BIT) || > > \ > > + defined(CONFIG_NDS32) || defined(CONFIG_NIOS) || > > \ > > + defined(CONFIG_OPENRISC) || defined(CONFIG_PPC32) || \ > > + defined(CONFIG_SUPERH) || defined(CONFIG_UML) || > > defined(CONFIG_UNICORE32) > > +#undef CONFIG_ARCH_HAS_CMPXCHG64 > > +#else > > +#define CONFIG_ARCH_HAS_CMPXCHG64 > > +#endif > > I think the encapsulation of blk_mq_set_rq_state() in terms of lock vs > cmpxchg64 is fine, but this does not belong in the block layer. > > > + /* Only used if CONFIG_ARCH_HAS_CMPXCHG64 is not defined. */ > > + spinlock_t das_lock; > > Let's put it under an if defined() then. OK, I will move CONFIG_ARCH_HAS_CMPXCHG64 into arch/*/Kconfig and I will surround spinlock_t das_lock with #if defined(...) / #endif. Thanks, Bart.
