From: Christoph Hellwig <hch@lst.de>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
Keith Busch <keith.busch@intel.com>,
linux-rdma@vger.kernel.org, linux-nvme@lists.infradead.org,
linux-block@vger.kernel.org
Subject: Re: [PATCH v2 6/6] nvme-multipath: disable polling for underlying namespace request queue
Date: Thu, 13 Dec 2018 21:13:30 +0100 [thread overview]
Message-ID: <20181213201330.GD15478@lst.de> (raw)
In-Reply-To: <0c5c172f-9b34-3cba-37ab-a39ad9351da5@grimberg.me>
On Thu, Dec 13, 2018 at 08:14:57AM -0800, Sagi Grimberg wrote:
>>>> + if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
>>>> + bio->bi_opf &= ~REQ_HIPRI;
>>>> +
>>>
>>> Maybe we can simply check (q->queue_flags & (1 << QUEUE_FLAG_POLL)) and
>>> avoid the extra atomic operation in the host path?
>>>
>>> Would it make sense?
>>
>> test_bit is not usually implemented as an atomic operation.
>>
>> Take a look at e.g.
>>
>> arch/x86/include/asm/bitops.h:constant_test_bit()
>
> Ah.. But its still read from volatile argument so still more expensive?
I don't think the volatile should make a difference. I actually
compiled both versions and the test_bit version generates a movq + testl
insted of testb:
- movq 120(%rbx), %rdx # MEM[(const long unsigned int *)q_38 + 120B], _135
- testl $524288, %edx #, _135
+ testb $8, 122(%rbx) #, q_40->queue_flags
But actually generates a larger object:
36966 9470 88 46524 b5bc blk-core.o-opencode
36956 9470 88 46514 b5b2 blk-core.o-test-bit
No idea what is going there.
WARNING: multiple messages have this Message-ID (diff)
From: hch@lst.de (Christoph Hellwig)
Subject: [PATCH v2 6/6] nvme-multipath: disable polling for underlying namespace request queue
Date: Thu, 13 Dec 2018 21:13:30 +0100 [thread overview]
Message-ID: <20181213201330.GD15478@lst.de> (raw)
In-Reply-To: <0c5c172f-9b34-3cba-37ab-a39ad9351da5@grimberg.me>
On Thu, Dec 13, 2018@08:14:57AM -0800, Sagi Grimberg wrote:
>>>> + if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
>>>> + bio->bi_opf &= ~REQ_HIPRI;
>>>> +
>>>
>>> Maybe we can simply check (q->queue_flags & (1 << QUEUE_FLAG_POLL)) and
>>> avoid the extra atomic operation in the host path?
>>>
>>> Would it make sense?
>>
>> test_bit is not usually implemented as an atomic operation.
>>
>> Take a look at e.g.
>>
>> arch/x86/include/asm/bitops.h:constant_test_bit()
>
> Ah.. But its still read from volatile argument so still more expensive?
I don't think the volatile should make a difference. I actually
compiled both versions and the test_bit version generates a movq + testl
insted of testb:
- movq 120(%rbx), %rdx # MEM[(const long unsigned int *)q_38 + 120B], _135
- testl $524288, %edx #, _135
+ testb $8, 122(%rbx) #, q_40->queue_flags
But actually generates a larger object:
36966 9470 88 46524 b5bc blk-core.o-opencode
36956 9470 88 46514 b5b2 blk-core.o-test-bit
No idea what is going there.
next prev parent reply other threads:[~2018-12-13 20:13 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-13 6:38 [PATCH v2 0/6] restore nvme-rdma polling Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 6:38 ` [PATCH v2 1/6] block: introduce blk_execute_rq_polled Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 8:24 ` Christoph Hellwig
2018-12-13 8:24 ` Christoph Hellwig
2018-12-13 15:23 ` Steve Wise
2018-12-13 15:23 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 2/6] nvme-core: allow __nvme_submit_sync_cmd to poll Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 8:27 ` Christoph Hellwig
2018-12-13 8:27 ` Christoph Hellwig
2018-12-13 15:19 ` Steve Wise
2018-12-13 15:19 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 3/6] nvme-fabrics: allow nvmf_connect_io_queue " Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 8:27 ` Christoph Hellwig
2018-12-13 8:27 ` Christoph Hellwig
2018-12-13 15:25 ` Steve Wise
2018-12-13 15:25 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 4/6] nvme-fabrics: allow user to pass in nr_poll_queues Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 15:26 ` Steve Wise
2018-12-13 15:26 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 5/6] nvme-rdma: implement polling queue map Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 15:28 ` Steve Wise
2018-12-13 15:28 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 6/6] nvme-multipath: disable polling for underlying namespace request queue Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 8:31 ` Christoph Hellwig
2018-12-13 8:31 ` Christoph Hellwig
2018-12-13 15:49 ` Sagi Grimberg
2018-12-13 15:49 ` Sagi Grimberg
2018-12-13 15:52 ` Christoph Hellwig
2018-12-13 15:52 ` Christoph Hellwig
2018-12-13 16:14 ` Sagi Grimberg
2018-12-13 16:14 ` Sagi Grimberg
2018-12-13 20:13 ` Christoph Hellwig [this message]
2018-12-13 20:13 ` Christoph Hellwig
2018-12-13 15:28 ` Steve Wise
2018-12-13 15:28 ` Steve Wise
2018-12-13 6:38 ` [PATCH v2 nvme-cli 7/6] fabrics: pass in number of polling queues Sagi Grimberg
2018-12-13 6:38 ` Sagi Grimberg
2018-12-13 15:29 ` Steve Wise
2018-12-13 15:29 ` Steve Wise
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181213201330.GD15478@lst.de \
--to=hch@lst.de \
--cc=axboe@kernel.dk \
--cc=keith.busch@intel.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-rdma@vger.kernel.org \
--cc=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.