10:49 p.m.
Hi Ming,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4 next-20201116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: powerpc64-randconfig-r026-20201116 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
#
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no
member named 'device_busy' in 'struct scsi_device'
sdev_busy = atomic_read(&scmd->device->device_busy);
~~~~~~~~~~~~ ^
1 error generated.
vim +365 drivers/scsi/megaraid/megaraid_sas_fusion.c
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 353
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 354 static inline void
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 355 megasas_get_msix_index(struct
megasas_instance *instance,
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 356 struct scsi_cmnd *scmd,
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 357 struct megasas_cmd_fusion *cmd,
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 358 u8 data_arms)
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 359 {
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 360 int sdev_busy;
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 361
103fbf8e4020845 Kashyap Desai 2020-08-19 362 /* TBD - if sml remove device_busy in
future, driver
103fbf8e4020845 Kashyap Desai 2020-08-19 363 * should track counter in internal
structure.
103fbf8e4020845 Kashyap Desai 2020-08-19 364 */
103fbf8e4020845 Kashyap Desai 2020-08-19 @365 sdev_busy =
atomic_read(&scmd->device->device_busy);
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 366
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 367 if (instance->perf_mode ==
MR_BALANCED_PERF_MODE &&
103fbf8e4020845 Kashyap Desai 2020-08-19 368 sdev_busy > (data_arms *
MR_DEVICE_HIGH_IOPS_DEPTH)) {
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 369 cmd->request_desc->SCSIIO.MSIxIndex
=
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 370 mega_mod64((atomic64_add_return(1,
&instance->high_iops_outstanding) /
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 371 MR_HIGH_IOPS_BATCH_COUNT),
instance->low_latency_index_start);
103fbf8e4020845 Kashyap Desai 2020-08-19 372 } else if (instance->msix_load_balance)
{
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 373 cmd->request_desc->SCSIIO.MSIxIndex
=
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 374 (mega_mod64(atomic64_add_return(1,
&instance->total_io_count),
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 375 instance->msix_vectors));
103fbf8e4020845 Kashyap Desai 2020-08-19 376 } else if
(instance->host->nr_hw_queues > 1) {
103fbf8e4020845 Kashyap Desai 2020-08-19 377 u32 tag =
blk_mq_unique_tag(scmd->request);
103fbf8e4020845 Kashyap Desai 2020-08-19 378
103fbf8e4020845 Kashyap Desai 2020-08-19 379 cmd->request_desc->SCSIIO.MSIxIndex
= blk_mq_unique_tag_to_hwq(tag) +
103fbf8e4020845 Kashyap Desai 2020-08-19 380 instance->low_latency_index_start;
103fbf8e4020845 Kashyap Desai 2020-08-19 381 } else {
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 382 cmd->request_desc->SCSIIO.MSIxIndex
=
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 383
instance->reply_map[raw_smp_processor_id()];
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 384 }
103fbf8e4020845 Kashyap Desai 2020-08-19 385 }
4d1634b8d12ecb8 Anand Lodnoor 2020-01-14 386
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
1:35 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
Hello Kashyap & Sumanesh,
On Mon, Nov 16, 2020 at 07:49:31PM +0800, kernel test robot wrote:
Hi Ming,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4 next-20201116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: powerpc64-randconfig-r026-20201116 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
#
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
>> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no member named
'device_busy' in 'struct scsi_device'
sdev_busy = atomic_read(&scmd->device->device_busy);
This new reference to sdev->device_busy is added by recent shared host
tag patch, and according to the comment, you may have planed to convert into
one megaraid internal counter.
/* TBD - if sml remove device_busy in future, driver
* should track counter in internal structure.
*/
So can you post one patch? And I am happy to fold it into this series.
Thanks,
Ming
6:15 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
Hey Ming,
On 11/18/20 3:35 AM, Ming Lei wrote:
Hello Kashyap & Sumanesh,
On Mon, Nov 16, 2020 at 07:49:31PM +0800, kernel test robot wrote:
> Hi Ming,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on block/for-next]
> [also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4 next-20201116]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
> base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-next
> config: powerpc64-randconfig-r026-20201116 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
> reproduce (this is a W=1 build):
> wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc64 cross compiling tool for clang build
> # apt-get install binutils-powerpc64-linux-gnu
> #
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
> git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp(a)intel.com>
>
> All errors (new ones prefixed by >>):
>
>>> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no member named
'device_busy' in 'struct scsi_device'
> sdev_busy = atomic_read(&scmd->device->device_busy);
This new reference to sdev->device_busy is added by recent shared host
tag patch, and according to the comment, you may have planed to convert into
one megaraid internal counter.
/* TBD - if sml remove device_busy in future, driver
* should track counter in internal structure.
*/
So can you post one patch? And I am happy to fold it into this series.
Seeing that we already have the accessor 'scsi_device_busy()' it's
probably easier to just use that and not fiddle with driver internals.
See the attached patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare(a)suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
6:44 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
On Wed, Nov 18, 2020 at 08:15:47AM +0100, Hannes Reinecke wrote:
Hey Ming,
On 11/18/20 3:35 AM, Ming Lei wrote:
> Hello Kashyap & Sumanesh,
>
> On Mon, Nov 16, 2020 at 07:49:31PM +0800, kernel test robot wrote:
> > Hi Ming,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on block/for-next]
> > [also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4
next-20201116]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-next
> > config: powerpc64-randconfig-r026-20201116 (attached as .config)
> > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
> > reproduce (this is a W=1 build):
> > wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # install powerpc64 cross compiling tool for clang build
> > # apt-get install binutils-powerpc64-linux-gnu
> > #
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
> > git remote add linux-review https://github.com/0day-ci/linux
> > git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
> > git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
> > # save the attached .config to linux build tree
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
ARCH=powerpc64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp(a)intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> > > > drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no member
named 'device_busy' in 'struct scsi_device'
> > sdev_busy = atomic_read(&scmd->device->device_busy);
>
> This new reference to sdev->device_busy is added by recent shared host
> tag patch, and according to the comment, you may have planed to convert into
> one megaraid internal counter.
>
> /* TBD - if sml remove device_busy in future, driver
> * should track counter in internal structure.
> */
>
> So can you post one patch? And I am happy to fold it into this series.
>
Seeing that we already have the accessor 'scsi_device_busy()' it's probably
easier to just use that and not fiddle with driver internals.
See the attached patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare(a)suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
From d8fa5e61187dbe851b8da9c65a5df5ec5809f8ea Mon Sep 17 00:00:00
2001
From: Hannes Reinecke <hare(a)suse.de>
Date: Wed, 18 Nov 2020 08:08:41 +0100
Subject: [PATCH] megaraid_sas: use scsi_device_busy() instead of direct access
to atomic counter
It's always a bad style to access structure internals, especially if
there is an accessor for it. So convert to use scsi_device_busy()
intead of accessing the atomic counter directly.
Cc: Kashyap Desai <kashyap.desai(a)broadcom.com>
Cc: Sumanesh Samanta <sumanesh.samanta(a)broadcom.com>
Signed-off-by: Hannes Reinecke <hare(a)suse.de>
---
drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index fd607287608e..272ff123bc6b 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -362,7 +362,7 @@ megasas_get_msix_index(struct megasas_instance *instance,
/* TBD - if sml remove device_busy in future, driver
* should track counter in internal structure.
*/
- sdev_busy = atomic_read(&scmd->device->device_busy);
+ sdev_busy = scsi_device_busy(scmd->device);
megasas_get_msix_index() is called in .queuecommand() path,
scsi_device_busy() might take more cycles since it has to iterate over
each sbitmap words, especially when the sbitmap depth is high.
I'd suggest Kashyap/Sumanesh to check if there is better way to
deal with it. If not, scsi_device_busy() should be fine.
Thanks,
Ming
8:15 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
On 11/18/20 8:44 AM, Ming Lei wrote:
On Wed, Nov 18, 2020 at 08:15:47AM +0100, Hannes Reinecke wrote:
> Hey Ming,
>
> On 11/18/20 3:35 AM, Ming Lei wrote:
>> Hello Kashyap & Sumanesh,
>>
>> On Mon, Nov 16, 2020 at 07:49:31PM +0800, kernel test robot wrote:
>>> Hi Ming,
>>>
>>> Thank you for the patch! Yet something to improve:
>>>
>>> [auto build test ERROR on block/for-next]
>>> [also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4
next-20201116]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented
in
>>> https://git-scm.com/docs/git-format-patch]
>>>
>>> url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
for-next
>>> config: powerpc64-randconfig-r026-20201116 (attached as .config)
>>> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
>>> reproduce (this is a W=1 build):
>>> wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
>>> chmod +x ~/bin/make.cross
>>> # install powerpc64 cross compiling tool for clang build
>>> # apt-get install binutils-powerpc64-linux-gnu
>>> #
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
>>> git remote add linux-review https://github.com/0day-ci/linux
>>> git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
>>> git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
>>> # save the attached .config to linux build tree
>>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
ARCH=powerpc64
>>>
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp(a)intel.com>
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no member
named 'device_busy' in 'struct scsi_device'
>>> sdev_busy = atomic_read(&scmd->device->device_busy);
>>
>> This new reference to sdev->device_busy is added by recent shared host
>> tag patch, and according to the comment, you may have planed to convert into
>> one megaraid internal counter.
>>
>> /* TBD - if sml remove device_busy in future, driver
>> * should track counter in internal structure.
>> */
>>
>> So can you post one patch? And I am happy to fold it into this series.
>>
> Seeing that we already have the accessor 'scsi_device_busy()' it's
probably
> easier to just use that and not fiddle with driver internals.
> See the attached patch.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> hare(a)suse.de +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
> From d8fa5e61187dbe851b8da9c65a5df5ec5809f8ea Mon Sep 17 00:00:00 2001
> From: Hannes Reinecke <hare(a)suse.de>
> Date: Wed, 18 Nov 2020 08:08:41 +0100
> Subject: [PATCH] megaraid_sas: use scsi_device_busy() instead of direct access
> to atomic counter
>
> It's always a bad style to access structure internals, especially if
> there is an accessor for it. So convert to use scsi_device_busy()
> intead of accessing the atomic counter directly.
>
> Cc: Kashyap Desai <kashyap.desai(a)broadcom.com>
> Cc: Sumanesh Samanta <sumanesh.samanta(a)broadcom.com>
> Signed-off-by: Hannes Reinecke <hare(a)suse.de>
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index fd607287608e..272ff123bc6b 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -362,7 +362,7 @@ megasas_get_msix_index(struct megasas_instance *instance,
> /* TBD - if sml remove device_busy in future, driver
> * should track counter in internal structure.
> */
> - sdev_busy = atomic_read(&scmd->device->device_busy);
> + sdev_busy = scsi_device_busy(scmd->device);
megasas_get_msix_index() is called in .queuecommand() path,
scsi_device_busy() might take more cycles since it has to iterate over
each sbitmap words, especially when the sbitmap depth is high.
I'd suggest Kashyap/Sumanesh to check if there is better way to
deal with it. If not, scsi_device_busy() should be fine.
I guess this whole codepath will become obsolete if and when support for
polling queues / io_uring will be implemented for megaraid_sas.
This whole section deals with spreading the load over several hardware
queues once the dedicated one is at risk of being congested.
But this is only required if someone want to reach high IOPS; so if we
have poll/io_uring support there won't be a need for this anymore.
Or that's the theory, at least :-)
But the patch should be good enough for now to get your patchset in.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare(a)suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
5:20 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp(a)intel.com>
>
> All errors (new ones prefixed by >>):
>
> >> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no
member
named 'device_busy' in 'struct scsi_device'
> sdev_busy = atomic_read(&scmd->device->device_busy);
This new reference to sdev->device_busy is added by recent shared host
tag
patch, and according to the comment, you may have planed to convert
into
one megaraid internal counter.
/* TBD - if sml remove device_busy in future, driver
* should track counter in internal structure.
*/
So can you post one patch? And I am happy to fold it into this series.
Ming - Please find the patch for megaraid_sas driver -
I have used helper inline function just for inter-operability with older
kernel to support in our out of box driver.
This way it will be easy for us to replace helper function as per kernel
version check.
Subject: [PATCH] megaraid_sas: replace sdev_busy with local counter
---
drivers/scsi/megaraid/megaraid_sas.h | 2 ++
drivers/scsi/megaraid/megaraid_sas_fusion.c | 34 ++++++++++++++++++---
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.h
b/drivers/scsi/megaraid/megaraid_sas.h
index 0f808d63580e..0c6a56b24c6e 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2019,10 +2019,12 @@ union megasas_frame {
* struct MR_PRIV_DEVICE - sdev private hostdata
* @is_tm_capable: firmware managed tm_capable flag
* @tm_busy: TM request is in progress
+ * @sdev_priv_busy: pending command per sdev
*/
struct MR_PRIV_DEVICE {
bool is_tm_capable;
bool tm_busy;
+ atomic_t sdev_priv_busy;
atomic_t r1_ldio_hint;
u8 interface_type;
u8 task_abort_tmo;
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index fd607287608e..e813ea0ad8b7 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -220,6 +220,32 @@ megasas_clear_intr_fusion(struct megasas_instance
*instance)
return 1;
}
+static inline void
+megasas_sdev_busy_inc(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_inc(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline void
+megasas_sdev_busy_dec(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_dec(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline int
+megasas_sdev_busy_read(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ return atomic_read(&mr_device_priv_data->sdev_priv_busy);
+}
+
+
/**
* megasas_get_cmd_fusion - Get a command from the free pool
* @instance: Adapter soft state
@@ -359,10 +385,7 @@ megasas_get_msix_index(struct megasas_instance
*instance,
{
int sdev_busy;
- /* TBD - if sml remove device_busy in future, driver
- * should track counter in internal structure.
- */
- sdev_busy = atomic_read(&scmd->device->device_busy);
+ sdev_busy = megasas_sdev_busy_read(scmd);
if (instance->perf_mode == MR_BALANCED_PERF_MODE &&
sdev_busy > (data_arms * MR_DEVICE_HIGH_IOPS_DEPTH)) {
@@ -3390,6 +3413,7 @@ megasas_build_and_issue_cmd_fusion(struct
megasas_instance *instance,
* Issue the command to the FW
*/
+ megasas_sdev_busy_inc(scmd);
megasas_fire_cmd_fusion(instance, req_desc);
if (r1_cmd)
@@ -3450,6 +3474,7 @@ megasas_complete_r1_command(struct megasas_instance
*instance,
scmd_local->SCp.ptr = NULL;
megasas_return_cmd_fusion(instance, cmd);
scsi_dma_unmap(scmd_local);
+ megasas_sdev_busy_dec(scmd_local);
scmd_local->scsi_done(scmd_local);
}
}
@@ -3550,6 +3575,7 @@ complete_cmd_fusion(struct megasas_instance
*instance, u32 MSIxIndex,
scmd_local->SCp.ptr = NULL;
megasas_return_cmd_fusion(instance,
cmd_fusion);
scsi_dma_unmap(scmd_local);
+ megasas_sdev_busy_dec(scmd_local);
scmd_local->scsi_done(scmd_local);
} else /* Optimal VD - R1 FP command completion.
*/
megasas_complete_r1_command(instance,
cmd_fusion);
--
2.18.1
Thanks,
Ming
5:29 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
> From: Hannes Reinecke <hare(a)suse.de>
> Date: Wed, 18 Nov 2020 08:08:41 +0100
> Subject: [PATCH] megaraid_sas: use scsi_device_busy() instead of
> direct access to atomic counter
>
> It's always a bad style to access structure internals, especially if
> there is an accessor for it. So convert to use scsi_device_busy()
> intead of accessing the atomic counter directly.
>
> Cc: Kashyap Desai <kashyap.desai(a)broadcom.com>
> Cc: Sumanesh Samanta <sumanesh.samanta(a)broadcom.com>
> Signed-off-by: Hannes Reinecke <hare(a)suse.de>
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index fd607287608e..272ff123bc6b 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -362,7 +362,7 @@ megasas_get_msix_index(struct megasas_instance
*instance,
> /* TBD - if sml remove device_busy in future, driver
> * should track counter in internal structure.
> */
> - sdev_busy = atomic_read(&scmd->device->device_busy);
> + sdev_busy = scsi_device_busy(scmd->device);
megasas_get_msix_index() is called in .queuecommand() path,
scsi_device_busy() might take more cycles since it has to iterate over
each
sbitmap words, especially when the sbitmap depth is high.
I'd suggest Kashyap/Sumanesh to check if there is better way to deal
with it.
If
not, scsi_device_busy() should be fine.
Scsi_device_busy() add significant amount of overhead which will be
visible in terms of reduced IOPS and high CPU cycle. I tested it earlier
and noticed regression in performance.
I posted megaraid_sas driver patch which will use internal per sdev
outstanding similar to legacy sdev_busy counter.
Kashyap
Thanks,
Ming
5:30 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
On Wed, Nov 18, 2020 at 10:15:19AM +0100, Hannes Reinecke wrote:
On 11/18/20 8:44 AM, Ming Lei wrote:
> On Wed, Nov 18, 2020 at 08:15:47AM +0100, Hannes Reinecke wrote:
> > Hey Ming,
> >
> > On 11/18/20 3:35 AM, Ming Lei wrote:
> > > Hello Kashyap & Sumanesh,
> > >
> > > On Mon, Nov 16, 2020 at 07:49:31PM +0800, kernel test robot wrote:
> > > > Hi Ming,
> > > >
> > > > Thank you for the patch! Yet something to improve:
> > > >
> > > > [auto build test ERROR on block/for-next]
> > > > [also build test ERROR on mkp-scsi/for-next scsi/for-next v5.10-rc4
next-20201116]
> > > > [If your patch is applied to the wrong git tree, kindly drop us a
note.
> > > > And when submitting patch, we suggest to use '--base' as
documented in
> > > > https://git-scm.com/docs/git-format-patch]
> > > >
> > > > url:
https://github.com/0day-ci/linux/commits/Ming-Lei/blk-mq-scsi-tracking-de...
> > > > base:
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> > > > config: powerpc64-randconfig-r026-20201116 (attached as .config)
> > > > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
c044709b8fbea2a9a375e4173a6bd735f6866c0c)
> > > > reproduce (this is a W=1 build):
> > > > wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
> > > > chmod +x ~/bin/make.cross
> > > > # install powerpc64 cross compiling tool for clang build
> > > > # apt-get install binutils-powerpc64-linux-gnu
> > > > #
https://github.com/0day-ci/linux/commit/cc286ae987be50d7b8e152cc80a5ccaa8...
> > > > git remote add linux-review
https://github.com/0day-ci/linux
> > > > git fetch --no-tags linux-review
Ming-Lei/blk-mq-scsi-tracking-device-queue-depth-via-sbitmap/20201116-171449
> > > > git checkout cc286ae987be50d7b8e152cc80a5ccaa8682e3ff
> > > > # save the attached .config to linux build tree
> > > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
ARCH=powerpc64
> > > >
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp(a)intel.com>
> > > >
> > > > All errors (new ones prefixed by >>):
> > > >
> > > > > > drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error:
no member named 'device_busy' in 'struct scsi_device'
> > > > sdev_busy =
atomic_read(&scmd->device->device_busy);
> > >
> > > This new reference to sdev->device_busy is added by recent shared host
> > > tag patch, and according to the comment, you may have planed to convert
into
> > > one megaraid internal counter.
> > >
> > > /* TBD - if sml remove device_busy in future, driver
> > > * should track counter in internal structure.
> > > */
> > >
> > > So can you post one patch? And I am happy to fold it into this series.
> > >
> > Seeing that we already have the accessor 'scsi_device_busy()' it's
probably
> > easier to just use that and not fiddle with driver internals.
> > See the attached patch.
> >
> > Cheers,
> >
> > Hannes
> > --
> > Dr. Hannes Reinecke Kernel Storage Architect
> > hare(a)suse.de +49 911 74053 688
> > SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
> > HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
>
> > From d8fa5e61187dbe851b8da9c65a5df5ec5809f8ea Mon Sep 17 00:00:00 2001
> > From: Hannes Reinecke <hare(a)suse.de>
> > Date: Wed, 18 Nov 2020 08:08:41 +0100
> > Subject: [PATCH] megaraid_sas: use scsi_device_busy() instead of direct access
> > to atomic counter
> >
> > It's always a bad style to access structure internals, especially if
> > there is an accessor for it. So convert to use scsi_device_busy()
> > intead of accessing the atomic counter directly.
> >
> > Cc: Kashyap Desai <kashyap.desai(a)broadcom.com>
> > Cc: Sumanesh Samanta <sumanesh.samanta(a)broadcom.com>
> > Signed-off-by: Hannes Reinecke <hare(a)suse.de>
> > ---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index fd607287608e..272ff123bc6b 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -362,7 +362,7 @@ megasas_get_msix_index(struct megasas_instance *instance,
> > /* TBD - if sml remove device_busy in future, driver
> > * should track counter in internal structure.
> > */
> > - sdev_busy = atomic_read(&scmd->device->device_busy);
> > + sdev_busy = scsi_device_busy(scmd->device);
>
> megasas_get_msix_index() is called in .queuecommand() path,
> scsi_device_busy() might take more cycles since it has to iterate over
> each sbitmap words, especially when the sbitmap depth is high.
>
> I'd suggest Kashyap/Sumanesh to check if there is better way to
> deal with it. If not, scsi_device_busy() should be fine.
>
I guess this whole codepath will become obsolete if and when support for
polling queues / io_uring will be implemented for megaraid_sas.
Not sure if it is related with iopoll which requires host tags. I understand
the code path for selecting msi index should be replaced with the following
simply if host tags is enabled:
if (instance->host->nr_hw_queues > 1) {
u32 tag = blk_mq_unique_tag(scmd->request);
cmd->request_desc->SCSIIO.MSIxIndex = blk_mq_unique_tag_to_hwq(tag)
+
instance->low_latency_index_start;
} else {
if (instance->perf_mode == MR_BALANCED_PERF_MODE)
...
else if (instance->msix_load_balance)
...
else
cmd->request_desc->SCSIIO.MSIxIndex =
instance->reply_map[raw_smp_processor_id()];
}
Otherwise there might be risk to trigger soft lockup in case of host tags.
sdev->device_busy is only required for MR_BALANCED_PERF_MODE, so your
patch can be adjusted to read the counter only for MR_BALANCED_PERF_MODE.
This whole section deals with spreading the load over several
hardware
queues once the dedicated one is at risk of being congested.
But this is only required if someone want to reach high IOPS; so if we have
poll/io_uring support there won't be a need for this anymore.
I understand poll is for low latency usage with extra cost of CPU utilization, and
iopoll can't replace irq based IO. But I may misunderstood your point.
Thanks,
Ming
5:34 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
On Thu, Nov 19, 2020 at 11:50:39AM +0530, Kashyap Desai wrote:
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp(a)intel.com>
> >
> > All errors (new ones prefixed by >>):
> >
> > >> drivers/scsi/megaraid/megaraid_sas_fusion.c:365:41: error: no
member
> named 'device_busy' in 'struct scsi_device'
> > sdev_busy = atomic_read(&scmd->device->device_busy);
>
> This new reference to sdev->device_busy is added by recent shared host
tag
> patch, and according to the comment, you may have planed to convert into
> one megaraid internal counter.
>
> /* TBD - if sml remove device_busy in future, driver
> * should track counter in internal structure.
> */
>
> So can you post one patch? And I am happy to fold it into this series.
Ming - Please find the patch for megaraid_sas driver -
I have used helper inline function just for inter-operability with older
kernel to support in our out of box driver.
This way it will be easy for us to replace helper function as per kernel
version check.
Subject: [PATCH] megaraid_sas: replace sdev_busy with local counter
---
drivers/scsi/megaraid/megaraid_sas.h | 2 ++
drivers/scsi/megaraid/megaraid_sas_fusion.c | 34 ++++++++++++++++++---
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.h
b/drivers/scsi/megaraid/megaraid_sas.h
index 0f808d63580e..0c6a56b24c6e 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2019,10 +2019,12 @@ union megasas_frame {
* struct MR_PRIV_DEVICE - sdev private hostdata
* @is_tm_capable: firmware managed tm_capable flag
* @tm_busy: TM request is in progress
+ * @sdev_priv_busy: pending command per sdev
*/
struct MR_PRIV_DEVICE {
bool is_tm_capable;
bool tm_busy;
+ atomic_t sdev_priv_busy;
atomic_t r1_ldio_hint;
u8 interface_type;
u8 task_abort_tmo;
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index fd607287608e..e813ea0ad8b7 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -220,6 +220,32 @@ megasas_clear_intr_fusion(struct megasas_instance
*instance)
return 1;
}
+static inline void
+megasas_sdev_busy_inc(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_inc(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline void
+megasas_sdev_busy_dec(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_dec(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline int
+megasas_sdev_busy_read(struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ return atomic_read(&mr_device_priv_data->sdev_priv_busy);
+}
+
+
/**
* megasas_get_cmd_fusion - Get a command from the free pool
* @instance: Adapter soft state
@@ -359,10 +385,7 @@ megasas_get_msix_index(struct megasas_instance
*instance,
{
int sdev_busy;
- /* TBD - if sml remove device_busy in future, driver
- * should track counter in internal structure.
- */
- sdev_busy = atomic_read(&scmd->device->device_busy);
+ sdev_busy = megasas_sdev_busy_read(scmd);
The above is only used for MR_BALANCED_PERF_MODE, so maybe you can
skip inc/dec/read the counter for other perf mode.
Thanks,
Ming
6:09 p.m.
New subject: [PATCH V4 12/12] scsi: replace sdev->device_busy with sbitmap
> /**
> * megasas_get_cmd_fusion - Get a command from the free pool
> * @instance: Adapter soft state
> @@ -359,10 +385,7 @@ megasas_get_msix_index(struct megasas_instance
> *instance, {
> int sdev_busy;
>
> - /* TBD - if sml remove device_busy in future, driver
> - * should track counter in internal structure.
> - */
> - sdev_busy = atomic_read(&scmd->device->device_busy);
> + sdev_busy = megasas_sdev_busy_read(scmd);
The above is only used for MR_BALANCED_PERF_MODE, so maybe you can
skip inc/dec/read the counter for other perf mode.
Agree. I have created V2.
Subject: [PATCH] megaraid_sas: v2 replace sdev_busy with local counter
use local tracking of per sdev outstanding command since sdev_busy in
SML is improved for peformance reason using sbitmap (earlier it was
atomic variable).
Signed-off-by: Kashyap Desai <kashyap.desai(a)broadcom.com>
---
drivers/scsi/megaraid/megaraid_sas.h | 2 +
drivers/scsi/megaraid/megaraid_sas_fusion.c | 51 +++++++++++++++++----
2 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas.h
b/drivers/scsi/megaraid/megaraid_sas.h
index 0f808d63580e..0c6a56b24c6e 100644
--- a/drivers/scsi/megaraid/megaraid_sas.h
+++ b/drivers/scsi/megaraid/megaraid_sas.h
@@ -2019,10 +2019,12 @@ union megasas_frame {
* struct MR_PRIV_DEVICE - sdev private hostdata
* @is_tm_capable: firmware managed tm_capable flag
* @tm_busy: TM request is in progress
+ * @sdev_priv_busy: pending command per sdev
*/
struct MR_PRIV_DEVICE {
bool is_tm_capable;
bool tm_busy;
+ atomic_t sdev_priv_busy;
atomic_t r1_ldio_hint;
u8 interface_type;
u8 task_abort_tmo;
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index fd607287608e..c630404cbb2d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -220,6 +220,44 @@ megasas_clear_intr_fusion(struct megasas_instance
*instance)
return 1;
}
+static inline void
+megasas_sdev_busy_inc(struct megasas_instance *instance,
+ struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ if (instance->perf_mode != MR_BALANCED_PERF_MODE)
+ return;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_inc(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline void
+megasas_sdev_busy_dec(struct megasas_instance *instance,
+ struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ if (instance->perf_mode != MR_BALANCED_PERF_MODE)
+ return;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ atomic_dec(&mr_device_priv_data->sdev_priv_busy);
+}
+static inline int
+megasas_sdev_busy_read(struct megasas_instance *instance,
+ struct scsi_cmnd *scmd)
+{
+ struct MR_PRIV_DEVICE *mr_device_priv_data;
+
+ if (instance->perf_mode != MR_BALANCED_PERF_MODE)
+ return 0;
+
+ mr_device_priv_data = scmd->device->hostdata;
+ return atomic_read(&mr_device_priv_data->sdev_priv_busy);
+}
+
+
/**
* megasas_get_cmd_fusion - Get a command from the free pool
* @instance: Adapter soft state
@@ -357,15 +395,9 @@ megasas_get_msix_index(struct megasas_instance
*instance,
struct megasas_cmd_fusion *cmd,
u8 data_arms)
{
- int sdev_busy;
-
- /* TBD - if sml remove device_busy in future, driver
- * should track counter in internal structure.
- */
- sdev_busy = atomic_read(&scmd->device->device_busy);
-
if (instance->perf_mode == MR_BALANCED_PERF_MODE &&
- sdev_busy > (data_arms * MR_DEVICE_HIGH_IOPS_DEPTH)) {
+ (megasas_sdev_busy_read(instance, scmd) >
+ (data_arms * MR_DEVICE_HIGH_IOPS_DEPTH))) {
cmd->request_desc->SCSIIO.MSIxIndex =
mega_mod64((atomic64_add_return(1,
&instance->high_iops_outstanding) /
MR_HIGH_IOPS_BATCH_COUNT),
instance->low_latency_index_start);
@@ -3390,6 +3422,7 @@ megasas_build_and_issue_cmd_fusion(struct
megasas_instance *instance,
* Issue the command to the FW
*/
+ megasas_sdev_busy_inc(instance, scmd);
megasas_fire_cmd_fusion(instance, req_desc);
if (r1_cmd)
@@ -3450,6 +3483,7 @@ megasas_complete_r1_command(struct megasas_instance
*instance,
scmd_local->SCp.ptr = NULL;
megasas_return_cmd_fusion(instance, cmd);
scsi_dma_unmap(scmd_local);
+ megasas_sdev_busy_dec(instance, scmd_local);
scmd_local->scsi_done(scmd_local);
}
}
@@ -3550,6 +3584,7 @@ complete_cmd_fusion(struct megasas_instance
*instance, u32 MSIxIndex,
scmd_local->SCp.ptr = NULL;
megasas_return_cmd_fusion(instance,
cmd_fusion);
scsi_dma_unmap(scmd_local);
+ megasas_sdev_busy_dec(instance,
scmd_local);
scmd_local->scsi_done(scmd_local);
} else /* Optimal VD - R1 FP command completion.
*/
megasas_complete_r1_command(instance,
cmd_fusion);
--
Thanks,
Ming
670
days inactive
673
days old
9 comments
4 participants
participants (4)
-
Hannes Reinecke -
Kashyap Desai -
kernel test robot -
Ming Lei