[GIT PULL] nvme update for Linux 4.14, take 2

[GIT PULL] nvme update for Linux 4.14, take 2

[Christoph Hellwig]

Hi Jens,

below is the current set of NVMe updates for Linux 4.14, now against your
postmerge branch, and with three more patches.

The biggest bit comes from Sagi and refactors the RDMA driver to prepare
for more code sharing in the setup and teardown path.  But we have various
features and bug fixes from a lot of people as well.

The following changes since commit cd996fb47c360320cf25ac9503c16de085ea9cfc:

  Merge tag 'v4.13-rc7' into for-4.14/block-postmerge (2017-08-28 13:00:44 -0600)

are available in the git repository at:

  git://git.infradead.org/nvme.git nvme-4.14

for you to fetch changes up to 1d5df6af8c7469f9ae3e66e7bed0782cfe4f95db:

  nvme: don't blindly overwrite identifiers on disk revalidate (2017-08-29 10:23:04 +0200)

----------------------------------------------------------------
Arnav Dawn (2):
      nvme: add support for FW activation without reset
      nvme: define NVME_NSID_ALL

Christoph Hellwig (6):
      nvmet: use NVME_NSID_ALL
      nvme: report more detailed status codes to the block layer
      nvme: allow calling nvme_change_ctrl_state from irq context
      nvme: remove unused struct nvme_ns fields
      nvme: remove nvme_revalidate_ns
      nvme: don't blindly overwrite identifiers on disk revalidate

Guan Junxiong (2):
      nvmet: fix the return error code of target if host is not allowed
      nvme-fabrics: log a warning if hostid is invalid

James Smart (2):
      nvme-fc: Reattach to localports on re-registration
      nvmet-fc: simplify sg list handling

Jan H. Sch�nherr (1):
      nvme: fix uninitialized prp2 value on small transfers

Johannes Thumshirn (2):
      nvmet-fcloop: remove ALL_OPTS define
      nvme-rdma: remove NVME_RDMA_MAX_SEGMENT_SIZE

Jon Derrick (1):
      nvme: add support for NVMe 1.3 Timestamp Feature

Martin K. Petersen (1):
      nvme: honor RTD3 Entry Latency for shutdowns

Martin Wilck (2):
      string.h: add memcpy_and_pad()
      nvmet: use memcpy_and_pad for identify sn/fr

Max Gurtovoy (3):
      nvme: add symbolic constants for CC identifiers
      nvme: rename AMS symbolic constants to fit specification
      nvme-rdma: Use unlikely macro in the fast path

Sagi Grimberg (13):
      nvme-rdma: move nvme_rdma_configure_admin_queue code location
      nvme: Add admin_tagset pointer to nvme_ctrl
      nvme-rdma: move tagset allocation to a dedicated routine
      nvme-rdma: disable the controller on resets
      nvme-rdma: don't free tagset on resets
      nvme-rdma: reuse configure/destroy_admin_queue
      nvme-rdma: introduce configure/destroy io queues
      nvme-rdma: stop queues instead of simply flipping their state
      nvme-rdma: rename nvme_rdma_init_queue to nvme_rdma_alloc_queue
      nvme-rdma: introduce nvme_rdma_start_queue
      nvme-rdma: cleanup error path in controller reset
      nvme-rdma: call ops->reg_read64 instead of nvmf_reg_read64
      nvme: fix identify namespace logging

 drivers/nvme/host/core.c        | 242 +++++++++++++----
 drivers/nvme/host/fabrics.c     |   1 +
 drivers/nvme/host/fc.c          | 145 ++++++++---
 drivers/nvme/host/nvme.h        |  10 +-
 drivers/nvme/host/pci.c         |   5 +-
 drivers/nvme/host/rdma.c        | 564 ++++++++++++++++++++--------------------
 drivers/nvme/target/admin-cmd.c |  16 +-
 drivers/nvme/target/configfs.c  |   2 +-
 drivers/nvme/target/core.c      |  15 +-
 drivers/nvme/target/fc.c        |  48 +---
 drivers/nvme/target/fcloop.c    |   3 -
 drivers/nvme/target/loop.c      |   1 +
 include/linux/nvme-fc-driver.h  |   2 +-
 include/linux/nvme.h            |  37 ++-
 include/linux/string.h          |  30 +++
 15 files changed, 677 insertions(+), 444 deletions(-)

[GIT PULL] nvme update for Linux 4.14, take 2

[Christoph Hellwig]

Hi Jens,

below is the current set of NVMe updates for Linux 4.14, now against your
postmerge branch, and with three more patches.

The biggest bit comes from Sagi and refactors the RDMA driver to prepare
for more code sharing in the setup and teardown path.  But we have various
features and bug fixes from a lot of people as well.

The following changes since commit cd996fb47c360320cf25ac9503c16de085ea9cfc:

  Merge tag 'v4.13-rc7' into for-4.14/block-postmerge (2017-08-28 13:00:44 -0600)

are available in the git repository at:

  git://git.infradead.org/nvme.git nvme-4.14

for you to fetch changes up to 1d5df6af8c7469f9ae3e66e7bed0782cfe4f95db:

  nvme: don't blindly overwrite identifiers on disk revalidate (2017-08-29 10:23:04 +0200)

----------------------------------------------------------------
Arnav Dawn (2):
      nvme: add support for FW activation without reset
      nvme: define NVME_NSID_ALL

Christoph Hellwig (6):
      nvmet: use NVME_NSID_ALL
      nvme: report more detailed status codes to the block layer
      nvme: allow calling nvme_change_ctrl_state from irq context
      nvme: remove unused struct nvme_ns fields
      nvme: remove nvme_revalidate_ns
      nvme: don't blindly overwrite identifiers on disk revalidate

Guan Junxiong (2):
      nvmet: fix the return error code of target if host is not allowed
      nvme-fabrics: log a warning if hostid is invalid

James Smart (2):
      nvme-fc: Reattach to localports on re-registration
      nvmet-fc: simplify sg list handling

Jan H. Sch?nherr (1):
      nvme: fix uninitialized prp2 value on small transfers

Johannes Thumshirn (2):
      nvmet-fcloop: remove ALL_OPTS define
      nvme-rdma: remove NVME_RDMA_MAX_SEGMENT_SIZE

Jon Derrick (1):
      nvme: add support for NVMe 1.3 Timestamp Feature

Martin K. Petersen (1):
      nvme: honor RTD3 Entry Latency for shutdowns

Martin Wilck (2):
      string.h: add memcpy_and_pad()
      nvmet: use memcpy_and_pad for identify sn/fr

Max Gurtovoy (3):
      nvme: add symbolic constants for CC identifiers
      nvme: rename AMS symbolic constants to fit specification
      nvme-rdma: Use unlikely macro in the fast path

Sagi Grimberg (13):
      nvme-rdma: move nvme_rdma_configure_admin_queue code location
      nvme: Add admin_tagset pointer to nvme_ctrl
      nvme-rdma: move tagset allocation to a dedicated routine
      nvme-rdma: disable the controller on resets
      nvme-rdma: don't free tagset on resets
      nvme-rdma: reuse configure/destroy_admin_queue
      nvme-rdma: introduce configure/destroy io queues
      nvme-rdma: stop queues instead of simply flipping their state
      nvme-rdma: rename nvme_rdma_init_queue to nvme_rdma_alloc_queue
      nvme-rdma: introduce nvme_rdma_start_queue
      nvme-rdma: cleanup error path in controller reset
      nvme-rdma: call ops->reg_read64 instead of nvmf_reg_read64
      nvme: fix identify namespace logging

 drivers/nvme/host/core.c        | 242 +++++++++++++----
 drivers/nvme/host/fabrics.c     |   1 +
 drivers/nvme/host/fc.c          | 145 ++++++++---
 drivers/nvme/host/nvme.h        |  10 +-
 drivers/nvme/host/pci.c         |   5 +-
 drivers/nvme/host/rdma.c        | 564 ++++++++++++++++++++--------------------
 drivers/nvme/target/admin-cmd.c |  16 +-
 drivers/nvme/target/configfs.c  |   2 +-
 drivers/nvme/target/core.c      |  15 +-
 drivers/nvme/target/fc.c        |  48 +---
 drivers/nvme/target/fcloop.c    |   3 -
 drivers/nvme/target/loop.c      |   1 +
 include/linux/nvme-fc-driver.h  |   2 +-
 include/linux/nvme.h            |  37 ++-
 include/linux/string.h          |  30 +++
 15 files changed, 677 insertions(+), 444 deletions(-)

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/29/2017 09:05 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> below is the current set of NVMe updates for Linux 4.14, now against your
> postmerge branch, and with three more patches.
> 
> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
> for more code sharing in the setup and teardown path.  But we have various
> features and bug fixes from a lot of people as well.

Pulled, thanks.

-- 
Jens Axboe

[GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/29/2017 09:05 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> below is the current set of NVMe updates for Linux 4.14, now against your
> postmerge branch, and with three more patches.
> 
> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
> for more code sharing in the setup and teardown path.  But we have various
> features and bug fixes from a lot of people as well.

Pulled, thanks.

-- 
Jens Axboe

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> Hi Jens,
>>
>> below is the current set of NVMe updates for Linux 4.14, now against your
>> postmerge branch, and with three more patches.
>>
>> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
>> for more code sharing in the setup and teardown path.  But we have various
>> features and bug fixes from a lot of people as well.
> 
> Pulled, thanks.

I just realized that patch:
--
commit d352ae205d8b05f3f7558d10f474d8436581b3e2
Author: Bart Van Assche <bart.vanassche@wdc.com>
Date:   Thu Aug 17 16:23:03 2017 -0700

     blk-mq: Make blk_mq_reinit_tagset() calls easier to read

     Since blk_mq_ops.reinit_request is only called from inside
     blk_mq_reinit_tagset(), make this function pointer an argument of
     blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
     This patch does not change any functionality but makes
     blk_mq_reinit_tagset() calls easier to read and to analyze.
--

Makes it impossible for me to move controller reset flow to
nvme-core without adding a trampoline (as the reinit_request
is transport specific)...

I'd really like to avoid adding more churn to the code refactoring
than it already has.

Given that this patch is not fixing anything should we revert it?

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> Hi Jens,
>>
>> below is the current set of NVMe updates for Linux 4.14, now against your
>> postmerge branch, and with three more patches.
>>
>> The biggest bit comes from Sagi and refactors the RDMA driver to prepare
>> for more code sharing in the setup and teardown path.  But we have various
>> features and bug fixes from a lot of people as well.
> 
> Pulled, thanks.

I just realized that patch:
--
commit d352ae205d8b05f3f7558d10f474d8436581b3e2
Author: Bart Van Assche <bart.vanassche at wdc.com>
Date:   Thu Aug 17 16:23:03 2017 -0700

     blk-mq: Make blk_mq_reinit_tagset() calls easier to read

     Since blk_mq_ops.reinit_request is only called from inside
     blk_mq_reinit_tagset(), make this function pointer an argument of
     blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
     This patch does not change any functionality but makes
     blk_mq_reinit_tagset() calls easier to read and to analyze.
--

Makes it impossible for me to move controller reset flow to
nvme-core without adding a trampoline (as the reinit_request
is transport specific)...

I'd really like to avoid adding more churn to the code refactoring
than it already has.

Given that this patch is not fixing anything should we revert it?

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

T24gV2VkLCAyMDE3LTA4LTMwIGF0IDE4OjEwICswMzAwLCBTYWdpIEdyaW1iZXJnIHdyb3RlOg0K
PiBJIGp1c3QgcmVhbGl6ZWQgdGhhdCBwYXRjaDoNCj4gLS0NCj4gY29tbWl0IGQzNTJhZTIwNWQ4
YjA1ZjNmNzU1OGQxMGY0NzRkODQzNjU4MWIzZTINCj4gQXV0aG9yOiBCYXJ0IFZhbiBBc3NjaGUg
PGJhcnQudmFuYXNzY2hlQHdkYy5jb20+DQo+IERhdGU6ICAgVGh1IEF1ZyAxNyAxNjoyMzowMyAy
MDE3IC0wNzAwDQo+IA0KPiAgICAgIGJsay1tcTogTWFrZSBibGtfbXFfcmVpbml0X3RhZ3NldCgp
IGNhbGxzIGVhc2llciB0byByZWFkDQo+IA0KPiAgICAgIFNpbmNlIGJsa19tcV9vcHMucmVpbml0
X3JlcXVlc3QgaXMgb25seSBjYWxsZWQgZnJvbSBpbnNpZGUNCj4gICAgICBibGtfbXFfcmVpbml0
X3RhZ3NldCgpLCBtYWtlIHRoaXMgZnVuY3Rpb24gcG9pbnRlciBhbiBhcmd1bWVudCBvZg0KPiAg
ICAgIGJsa19tcV9yZWluaXRfdGFnc2V0KCkgaW5zdGVhZCBvZiBhIG1lbWJlciBvZiBzdHJ1Y3Qg
YmxrX21xX29wcy4NCj4gICAgICBUaGlzIHBhdGNoIGRvZXMgbm90IGNoYW5nZSBhbnkgZnVuY3Rp
b25hbGl0eSBidXQgbWFrZXMNCj4gICAgICBibGtfbXFfcmVpbml0X3RhZ3NldCgpIGNhbGxzIGVh
c2llciB0byByZWFkIGFuZCB0byBhbmFseXplLg0KPiAtLQ0KPiANCj4gTWFrZXMgaXQgaW1wb3Nz
aWJsZSBmb3IgbWUgdG8gbW92ZSBjb250cm9sbGVyIHJlc2V0IGZsb3cgdG8NCj4gbnZtZS1jb3Jl
IHdpdGhvdXQgYWRkaW5nIGEgdHJhbXBvbGluZSAoYXMgdGhlIHJlaW5pdF9yZXF1ZXN0DQo+IGlz
IHRyYW5zcG9ydCBzcGVjaWZpYykuLi4NCg0KSGVsbG8gU2FnaSwNCg0KU29ycnkgYnV0IEkgZG91
YnQgdGhhdCB0aGF0IHBhdGNoIG1ha2VzIGl0ICJpbXBvc3NpYmxlIiB0byBtb3ZlIGNvbnRyb2xs
ZXINCnJlc2V0IGZsb3cgdG8gdGhlIE5WTWUgY29yZS4gVGhlcmUgYXJlIGFscmVhZHkgc2V2ZXJh
bCBmdW5jdGlvbiBwb2ludGVycyBpbg0KdGhlIG52bWVfY3RybF9vcHMgZGF0YSBzdHJ1Y3R1cmUg
YW5kIHRoZXJlIGlzIG9uZSBzdWNoIGRhdGEgc3RydWN0dXJlIHBlcg0KdHJhbnNwb3J0LiBIYWQg
eW91IGFscmVhZHkgY29uc2lkZXJlZCB0byBhZGQgYSBmdW5jdGlvbiBwb2ludGVyIHRvIHRoYXQN
CnN0cnVjdHVyZT8NCg0KQmFydC4=

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@18:10 +0300, Sagi Grimberg wrote:
> I just realized that patch:
> --
> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
> Author: Bart Van Assche <bart.vanassche at wdc.com>
> Date:   Thu Aug 17 16:23:03 2017 -0700
> 
>      blk-mq: Make blk_mq_reinit_tagset() calls easier to read
> 
>      Since blk_mq_ops.reinit_request is only called from inside
>      blk_mq_reinit_tagset(), make this function pointer an argument of
>      blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>      This patch does not change any functionality but makes
>      blk_mq_reinit_tagset() calls easier to read and to analyze.
> --
> 
> Makes it impossible for me to move controller reset flow to
> nvme-core without adding a trampoline (as the reinit_request
> is transport specific)...

Hello Sagi,

Sorry but I doubt that that patch makes it "impossible" to move controller
reset flow to the NVMe core. There are already several function pointers in
the nvme_ctrl_ops data structure and there is one such data structure per
transport. Had you already considered to add a function pointer to that
structure?

Bart.

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> I just realized that patch:
>> --
>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>> Author: Bart Van Assche <bart.vanassche@wdc.com>
>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>
>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>
>>       Since blk_mq_ops.reinit_request is only called from inside
>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>       This patch does not change any functionality but makes
>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>> --
>>
>> Makes it impossible for me to move controller reset flow to
>> nvme-core without adding a trampoline (as the reinit_request
>> is transport specific)...
> 
> Hello Sagi,
> 
> Sorry but I doubt that that patch makes it "impossible" to move controller
> reset flow to the NVMe core. There are already several function pointers in
> the nvme_ctrl_ops data structure and there is one such data structure per
> transport. Had you already considered to add a function pointer to that
> structure?

I have, that's the trampoline function that I was referring to, it feels
a bit funny to have aa nvme core function that would look like:

int nvme_reinit_request()
{
	return ctrl->ops->reinit_request()
}

I can easily do that, but doesn't it defeat the purpose
of blk_mq_ops?

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> I just realized that patch:
>> --
>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>> Author: Bart Van Assche <bart.vanassche at wdc.com>
>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>
>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>
>>       Since blk_mq_ops.reinit_request is only called from inside
>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>       This patch does not change any functionality but makes
>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>> --
>>
>> Makes it impossible for me to move controller reset flow to
>> nvme-core without adding a trampoline (as the reinit_request
>> is transport specific)...
> 
> Hello Sagi,
> 
> Sorry but I doubt that that patch makes it "impossible" to move controller
> reset flow to the NVMe core. There are already several function pointers in
> the nvme_ctrl_ops data structure and there is one such data structure per
> transport. Had you already considered to add a function pointer to that
> structure?

I have, that's the trampoline function that I was referring to, it feels
a bit funny to have aa nvme core function that would look like:

int nvme_reinit_request()
{
	return ctrl->ops->reinit_request()
}

I can easily do that, but doesn't it defeat the purpose
of blk_mq_ops?

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

T24gV2VkLCAyMDE3LTA4LTMwIGF0IDE4OjMzICswMzAwLCBTYWdpIEdyaW1iZXJnIHdyb3RlOg0K
PiA+ID4gSSBqdXN0IHJlYWxpemVkIHRoYXQgcGF0Y2g6DQo+ID4gPiAtLQ0KPiA+ID4gY29tbWl0
IGQzNTJhZTIwNWQ4YjA1ZjNmNzU1OGQxMGY0NzRkODQzNjU4MWIzZTINCj4gPiA+IEF1dGhvcjog
QmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KPiA+ID4gRGF0ZTogICBU
aHUgQXVnIDE3IDE2OjIzOjAzIDIwMTcgLTA3MDANCj4gPiA+IA0KPiA+ID4gICAgICAgYmxrLW1x
OiBNYWtlIGJsa19tcV9yZWluaXRfdGFnc2V0KCkgY2FsbHMgZWFzaWVyIHRvIHJlYWQNCj4gPiA+
IA0KPiA+ID4gICAgICAgU2luY2UgYmxrX21xX29wcy5yZWluaXRfcmVxdWVzdCBpcyBvbmx5IGNh
bGxlZCBmcm9tIGluc2lkZQ0KPiA+ID4gICAgICAgYmxrX21xX3JlaW5pdF90YWdzZXQoKSwgbWFr
ZSB0aGlzIGZ1bmN0aW9uIHBvaW50ZXIgYW4gYXJndW1lbnQgb2YNCj4gPiA+ICAgICAgIGJsa19t
cV9yZWluaXRfdGFnc2V0KCkgaW5zdGVhZCBvZiBhIG1lbWJlciBvZiBzdHJ1Y3QgYmxrX21xX29w
cy4NCj4gPiA+ICAgICAgIFRoaXMgcGF0Y2ggZG9lcyBub3QgY2hhbmdlIGFueSBmdW5jdGlvbmFs
aXR5IGJ1dCBtYWtlcw0KPiA+ID4gICAgICAgYmxrX21xX3JlaW5pdF90YWdzZXQoKSBjYWxscyBl
YXNpZXIgdG8gcmVhZCBhbmQgdG8gYW5hbHl6ZS4NCj4gPiA+IC0tDQo+ID4gPiANCj4gPiA+IE1h
a2VzIGl0IGltcG9zc2libGUgZm9yIG1lIHRvIG1vdmUgY29udHJvbGxlciByZXNldCBmbG93IHRv
DQo+ID4gPiBudm1lLWNvcmUgd2l0aG91dCBhZGRpbmcgYSB0cmFtcG9saW5lIChhcyB0aGUgcmVp
bml0X3JlcXVlc3QNCj4gPiA+IGlzIHRyYW5zcG9ydCBzcGVjaWZpYykuLi4NCj4gPiANCj4gPiBI
ZWxsbyBTYWdpLA0KPiA+IA0KPiA+IFNvcnJ5IGJ1dCBJIGRvdWJ0IHRoYXQgdGhhdCBwYXRjaCBt
YWtlcyBpdCAiaW1wb3NzaWJsZSIgdG8gbW92ZSBjb250cm9sbGVyDQo+ID4gcmVzZXQgZmxvdyB0
byB0aGUgTlZNZSBjb3JlLiBUaGVyZSBhcmUgYWxyZWFkeSBzZXZlcmFsIGZ1bmN0aW9uIHBvaW50
ZXJzIGluDQo+ID4gdGhlIG52bWVfY3RybF9vcHMgZGF0YSBzdHJ1Y3R1cmUgYW5kIHRoZXJlIGlz
IG9uZSBzdWNoIGRhdGEgc3RydWN0dXJlIHBlcg0KPiA+IHRyYW5zcG9ydC4gSGFkIHlvdSBhbHJl
YWR5IGNvbnNpZGVyZWQgdG8gYWRkIGEgZnVuY3Rpb24gcG9pbnRlciB0byB0aGF0DQo+ID4gc3Ry
dWN0dXJlPw0KPiANCj4gSSBoYXZlLCB0aGF0J3MgdGhlIHRyYW1wb2xpbmUgZnVuY3Rpb24gdGhh
dCBJIHdhcyByZWZlcnJpbmcgdG8sIGl0IGZlZWxzDQo+IGEgYml0IGZ1bm55IHRvIGhhdmUgYWEg
bnZtZSBjb3JlIGZ1bmN0aW9uIHRoYXQgd291bGQgbG9vayBsaWtlOg0KPiANCj4gaW50IG52bWVf
cmVpbml0X3JlcXVlc3QoKQ0KPiB7DQo+IAlyZXR1cm4gY3RybC0+b3BzLT5yZWluaXRfcmVxdWVz
dCgpDQo+IH0NCj4gDQo+IEkgY2FuIGVhc2lseSBkbyB0aGF0LCBidXQgZG9lc24ndCBpdCBkZWZl
YXQgdGhlIHB1cnBvc2Ugb2YgYmxrX21xX29wcz8NCg0KSSBkb24ndCB0aGluayBzby4gUmVxdWVz
dCByZWluaXRpYWxpemF0aW9uIGlzIGFuIE5WTWUgY29uY2VwdCB0aGF0IGlzIG5vdCB1c2VkDQpi
eSBhbnkgb3RoZXIgYmxvY2sgZHJpdmVyLCBzbyB3aHkgc2hvdWxkIHRoZSBwb2ludGVyIHRvIHRo
ZSByZWluaXRpYWxpemF0aW9uDQpmdW5jdGlvbiBleGlzdCBpbiBibGtfbXFfb3BzPw0KDQpCYXJ0
Lg==

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@18:33 +0300, Sagi Grimberg wrote:
> > > I just realized that patch:
> > > --
> > > commit d352ae205d8b05f3f7558d10f474d8436581b3e2
> > > Author: Bart Van Assche <bart.vanassche at wdc.com>
> > > Date:   Thu Aug 17 16:23:03 2017 -0700
> > > 
> > >       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
> > > 
> > >       Since blk_mq_ops.reinit_request is only called from inside
> > >       blk_mq_reinit_tagset(), make this function pointer an argument of
> > >       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
> > >       This patch does not change any functionality but makes
> > >       blk_mq_reinit_tagset() calls easier to read and to analyze.
> > > --
> > > 
> > > Makes it impossible for me to move controller reset flow to
> > > nvme-core without adding a trampoline (as the reinit_request
> > > is transport specific)...
> > 
> > Hello Sagi,
> > 
> > Sorry but I doubt that that patch makes it "impossible" to move controller
> > reset flow to the NVMe core. There are already several function pointers in
> > the nvme_ctrl_ops data structure and there is one such data structure per
> > transport. Had you already considered to add a function pointer to that
> > structure?
> 
> I have, that's the trampoline function that I was referring to, it feels
> a bit funny to have aa nvme core function that would look like:
> 
> int nvme_reinit_request()
> {
> 	return ctrl->ops->reinit_request()
> }
> 
> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?

I don't think so. Request reinitialization is an NVMe concept that is not used
by any other block driver, so why should the pointer to the reinitialization
function exist in blk_mq_ops?

Bart.

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/30/2017 09:46 AM, Bart Van Assche wrote:
> On Wed, 2017-08-30 at 18:33 +0300, Sagi Grimberg wrote:
>>>> I just realized that patch:
>>>> --
>>>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>>>> Author: Bart Van Assche <bart.vanassche@wdc.com>
>>>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>>>
>>>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>>>
>>>>       Since blk_mq_ops.reinit_request is only called from inside
>>>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>>>       This patch does not change any functionality but makes
>>>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>>>> --
>>>>
>>>> Makes it impossible for me to move controller reset flow to
>>>> nvme-core without adding a trampoline (as the reinit_request
>>>> is transport specific)...
>>>
>>> Hello Sagi,
>>>
>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>> reset flow to the NVMe core. There are already several function pointers in
>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>> transport. Had you already considered to add a function pointer to that
>>> structure?
>>
>> I have, that's the trampoline function that I was referring to, it feels
>> a bit funny to have aa nvme core function that would look like:
>>
>> int nvme_reinit_request()
>> {
>> 	return ctrl->ops->reinit_request()
>> }
>>
>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
> 
> I don't think so. Request reinitialization is an NVMe concept that is
> not used by any other block driver, so why should the pointer to the
> reinitialization function exist in blk_mq_ops?

The point of blk-mq is to provide all the functionality that drivers
need, even if it is for just a single driver. Functionality that can be
removed from drivers is good. The smaller/simpler we can make the
driver, the better off we are, even if that means adding a bit of
complexity to the core.

Obviously this is a case-by-case decision. For this particular case, I'm
happy with either solution.

-- 
Jens Axboe

[GIT PULL] nvme update for Linux 4.14, take 2

[Jens Axboe]

On 08/30/2017 09:46 AM, Bart Van Assche wrote:
> On Wed, 2017-08-30@18:33 +0300, Sagi Grimberg wrote:
>>>> I just realized that patch:
>>>> --
>>>> commit d352ae205d8b05f3f7558d10f474d8436581b3e2
>>>> Author: Bart Van Assche <bart.vanassche at wdc.com>
>>>> Date:   Thu Aug 17 16:23:03 2017 -0700
>>>>
>>>>       blk-mq: Make blk_mq_reinit_tagset() calls easier to read
>>>>
>>>>       Since blk_mq_ops.reinit_request is only called from inside
>>>>       blk_mq_reinit_tagset(), make this function pointer an argument of
>>>>       blk_mq_reinit_tagset() instead of a member of struct blk_mq_ops.
>>>>       This patch does not change any functionality but makes
>>>>       blk_mq_reinit_tagset() calls easier to read and to analyze.
>>>> --
>>>>
>>>> Makes it impossible for me to move controller reset flow to
>>>> nvme-core without adding a trampoline (as the reinit_request
>>>> is transport specific)...
>>>
>>> Hello Sagi,
>>>
>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>> reset flow to the NVMe core. There are already several function pointers in
>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>> transport. Had you already considered to add a function pointer to that
>>> structure?
>>
>> I have, that's the trampoline function that I was referring to, it feels
>> a bit funny to have aa nvme core function that would look like:
>>
>> int nvme_reinit_request()
>> {
>> 	return ctrl->ops->reinit_request()
>> }
>>
>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
> 
> I don't think so. Request reinitialization is an NVMe concept that is
> not used by any other block driver, so why should the pointer to the
> reinitialization function exist in blk_mq_ops?

The point of blk-mq is to provide all the functionality that drivers
need, even if it is for just a single driver. Functionality that can be
removed from drivers is good. The smaller/simpler we can make the
driver, the better off we are, even if that means adding a bit of
complexity to the core.

Obviously this is a case-by-case decision. For this particular case, I'm
happy with either solution.

-- 
Jens Axboe

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>>>> Hello Sagi,
>>>>
>>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>>> reset flow to the NVMe core. There are already several function pointers in
>>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>>> transport. Had you already considered to add a function pointer to that
>>>> structure?
>>>
>>> I have, that's the trampoline function that I was referring to, it feels
>>> a bit funny to have aa nvme core function that would look like:
>>>
>>> int nvme_reinit_request()
>>> {
>>> 	return ctrl->ops->reinit_request()
>>> }
>>>
>>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
>>
>> I don't think so. Request reinitialization is an NVMe concept that is
>> not used by any other block driver, so why should the pointer to the
>> reinitialization function exist in blk_mq_ops?
> 
> The point of blk-mq is to provide all the functionality that drivers
> need, even if it is for just a single driver. Functionality that can be
> removed from drivers is good. The smaller/simpler we can make the
> driver, the better off we are, even if that means adding a bit of
> complexity to the core.
> 
> Obviously this is a case-by-case decision. For this particular case, I'm
> happy with either solution.

If we get to choose, my preference would be to restore the old behavior
because currently existing nvme transports keep their internal ctrl
representation in the tagset->driver_data as they implement
init/exit_request. Bouncing in nvme core would require to change that
and always keep struct nvme_ctrl as the tagset->driver_data and convert
it on every handler...

Everything is doable but it seems like an unneeded hassle to me...

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>>>> Hello Sagi,
>>>>
>>>> Sorry but I doubt that that patch makes it "impossible" to move controller
>>>> reset flow to the NVMe core. There are already several function pointers in
>>>> the nvme_ctrl_ops data structure and there is one such data structure per
>>>> transport. Had you already considered to add a function pointer to that
>>>> structure?
>>>
>>> I have, that's the trampoline function that I was referring to, it feels
>>> a bit funny to have aa nvme core function that would look like:
>>>
>>> int nvme_reinit_request()
>>> {
>>> 	return ctrl->ops->reinit_request()
>>> }
>>>
>>> I can easily do that, but doesn't it defeat the purpose of blk_mq_ops?
>>
>> I don't think so. Request reinitialization is an NVMe concept that is
>> not used by any other block driver, so why should the pointer to the
>> reinitialization function exist in blk_mq_ops?
> 
> The point of blk-mq is to provide all the functionality that drivers
> need, even if it is for just a single driver. Functionality that can be
> removed from drivers is good. The smaller/simpler we can make the
> driver, the better off we are, even if that means adding a bit of
> complexity to the core.
> 
> Obviously this is a case-by-case decision. For this particular case, I'm
> happy with either solution.

If we get to choose, my preference would be to restore the old behavior
because currently existing nvme transports keep their internal ctrl
representation in the tagset->driver_data as they implement
init/exit_request. Bouncing in nvme core would require to change that
and always keep struct nvme_ctrl as the tagset->driver_data and convert
it on every handler...

Everything is doable but it seems like an unneeded hassle to me...

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

T24gV2VkLCAyMDE3LTA4LTMwIGF0IDE5OjA1ICswMzAwLCBTYWdpIEdyaW1iZXJnIHdyb3RlOg0K
PiBJZiB3ZSBnZXQgdG8gY2hvb3NlLCBteSBwcmVmZXJlbmNlIHdvdWxkIGJlIHRvIHJlc3RvcmUg
dGhlIG9sZCBiZWhhdmlvcg0KPiBiZWNhdXNlIGN1cnJlbnRseSBleGlzdGluZyBudm1lIHRyYW5z
cG9ydHMga2VlcCB0aGVpciBpbnRlcm5hbCBjdHJsDQo+IHJlcHJlc2VudGF0aW9uIGluIHRoZSB0
YWdzZXQtPmRyaXZlcl9kYXRhIGFzIHRoZXkgaW1wbGVtZW50DQo+IGluaXQvZXhpdF9yZXF1ZXN0
LiBCb3VuY2luZyBpbiBudm1lIGNvcmUgd291bGQgcmVxdWlyZSB0byBjaGFuZ2UgdGhhdA0KPiBh
bmQgYWx3YXlzIGtlZXAgc3RydWN0IG52bWVfY3RybCBhcyB0aGUgdGFnc2V0LT5kcml2ZXJfZGF0
YSBhbmQgY29udmVydA0KPiBpdCBvbiBldmVyeSBoYW5kbGVyLi4uDQo+IA0KPiBFdmVyeXRoaW5n
IGlzIGRvYWJsZSBidXQgaXQgc2VlbXMgbGlrZSBhbiB1bm5lZWRlZCBoYXNzbGUgdG8gbWUuLi4N
Cg0KU29ycnkgYnV0IEknbSBub3QgY29udmluY2VkIHRoYXQgaXQgd291bGQgYmUgbmVjZXNzYXJ5
IHRvIGNoYW5nZSB3aGF0DQp0YWdzZXQtPmRyaXZlcl9kYXRhIHBvaW50cyBhdC4gSG93IGFib3V0
IG1vdmluZyBibGtfbXFfcmVpbml0X3RhZ3NldCgpIGZyb20NCnRoZSBibG9jayBsYXllciBjb3Jl
IHRvIHRoZSBOVk1lIGNvcmUsIHRvIHJlbmFtZSBpdCBhbmQgdG8gcGFzcyBhIHBvaW50ZXINCnRv
IHRoZSBudm1lX2N0cmwgZGF0YSBzdHJ1Y3R1cmUgdG8gdGhhdCBmdW5jdGlvbiBpbnN0ZWFkIG9m
IG9ubHkgYmxvY2sgbGF5ZXINCmluZm9ybWF0aW9uPw0KDQpCYXJ0Lg==

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@19:05 +0300, Sagi Grimberg wrote:
> If we get to choose, my preference would be to restore the old behavior
> because currently existing nvme transports keep their internal ctrl
> representation in the tagset->driver_data as they implement
> init/exit_request. Bouncing in nvme core would require to change that
> and always keep struct nvme_ctrl as the tagset->driver_data and convert
> it on every handler...
> 
> Everything is doable but it seems like an unneeded hassle to me...

Sorry but I'm not convinced that it would be necessary to change what
tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
the block layer core to the NVMe core, to rename it and to pass a pointer
to the nvme_ctrl data structure to that function instead of only block layer
information?

Bart.

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> If we get to choose, my preference would be to restore the old behavior
>> because currently existing nvme transports keep their internal ctrl
>> representation in the tagset->driver_data as they implement
>> init/exit_request. Bouncing in nvme core would require to change that
>> and always keep struct nvme_ctrl as the tagset->driver_data and convert
>> it on every handler...
>>
>> Everything is doable but it seems like an unneeded hassle to me...
> 
> Sorry but I'm not convinced that it would be necessary to change what
> tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
> the block layer core to the NVMe core, to rename it and to pass a pointer
> to the nvme_ctrl data structure to that function instead of only block layer
> information?

That would mean that I need to open-code the tagset iteration in nvme
which does not feel like something a driver should do.

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> If we get to choose, my preference would be to restore the old behavior
>> because currently existing nvme transports keep their internal ctrl
>> representation in the tagset->driver_data as they implement
>> init/exit_request. Bouncing in nvme core would require to change that
>> and always keep struct nvme_ctrl as the tagset->driver_data and convert
>> it on every handler...
>>
>> Everything is doable but it seems like an unneeded hassle to me...
> 
> Sorry but I'm not convinced that it would be necessary to change what
> tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
> the block layer core to the NVMe core, to rename it and to pass a pointer
> to the nvme_ctrl data structure to that function instead of only block layer
> information?

That would mean that I need to open-code the tagset iteration in nvme
which does not feel like something a driver should do.

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

T24gV2VkLCAyMDE3LTA4LTMwIGF0IDE5OjQ3ICswMzAwLCBTYWdpIEdyaW1iZXJnIHdyb3RlOg0K
PiA+ID4gSWYgd2UgZ2V0IHRvIGNob29zZSwgbXkgcHJlZmVyZW5jZSB3b3VsZCBiZSB0byByZXN0
b3JlIHRoZSBvbGQgYmVoYXZpb3INCj4gPiA+IGJlY2F1c2UgY3VycmVudGx5IGV4aXN0aW5nIG52
bWUgdHJhbnNwb3J0cyBrZWVwIHRoZWlyIGludGVybmFsIGN0cmwNCj4gPiA+IHJlcHJlc2VudGF0
aW9uIGluIHRoZSB0YWdzZXQtPmRyaXZlcl9kYXRhIGFzIHRoZXkgaW1wbGVtZW50DQo+ID4gPiBp
bml0L2V4aXRfcmVxdWVzdC4gQm91bmNpbmcgaW4gbnZtZSBjb3JlIHdvdWxkIHJlcXVpcmUgdG8g
Y2hhbmdlIHRoYXQNCj4gPiA+IGFuZCBhbHdheXMga2VlcCBzdHJ1Y3QgbnZtZV9jdHJsIGFzIHRo
ZSB0YWdzZXQtPmRyaXZlcl9kYXRhIGFuZCBjb252ZXJ0DQo+ID4gPiBpdCBvbiBldmVyeSBoYW5k
bGVyLi4uDQo+ID4gPiANCj4gPiA+IEV2ZXJ5dGhpbmcgaXMgZG9hYmxlIGJ1dCBpdCBzZWVtcyBs
aWtlIGFuIHVubmVlZGVkIGhhc3NsZSB0byBtZS4uLg0KPiA+IA0KPiA+IFNvcnJ5IGJ1dCBJJ20g
bm90IGNvbnZpbmNlZCB0aGF0IGl0IHdvdWxkIGJlIG5lY2Vzc2FyeSB0byBjaGFuZ2Ugd2hhdA0K
PiA+IHRhZ3NldC0+ZHJpdmVyX2RhdGEgcG9pbnRzIGF0LiBIb3cgYWJvdXQgbW92aW5nIGJsa19t
cV9yZWluaXRfdGFnc2V0KCkgZnJvbQ0KPiA+IHRoZSBibG9jayBsYXllciBjb3JlIHRvIHRoZSBO
Vk1lIGNvcmUsIHRvIHJlbmFtZSBpdCBhbmQgdG8gcGFzcyBhIHBvaW50ZXINCj4gPiB0byB0aGUg
bnZtZV9jdHJsIGRhdGEgc3RydWN0dXJlIHRvIHRoYXQgZnVuY3Rpb24gaW5zdGVhZCBvZiBvbmx5
IGJsb2NrIGxheWVyDQo+ID4gaW5mb3JtYXRpb24/DQo+IA0KPiBUaGF0IHdvdWxkIG1lYW4gdGhh
dCBJIG5lZWQgdG8gb3Blbi1jb2RlIHRoZSB0YWdzZXQgaXRlcmF0aW9uIGluIG52bWUNCj4gd2hp
Y2ggZG9lcyBub3QgZmVlbCBsaWtlIHNvbWV0aGluZyBhIGRyaXZlciBzaG91bGQgZG8uDQoNCkhv
dyBhYm91dCByZW5hbWluZyBibGtfbXFfcmVpbml0X3RhZ3NldCgpIGludG8gYmxrX21xX3RhZ3Nl
dF9pdGVyKCkgYW5kDQp0byBtYWtlIHRoZSBhcmd1bWVudCBsaXN0IG9mIGJsa19tcV90YWdzZXRf
aXRlcigpIG1vcmUgc2ltaWxhciB0byB0aGF0IG9mDQpibGtfbXFfcXVldWVfdGFnX2J1c3lfaXRl
cigpIHN1Y2ggdGhhdCBjYWxsZXJzIG9mIGJsa19tcV90YWdzZXRfaXRlcigpDQpjYW4gcGFzcyBh
IHBvaW50ZXIgdG8gYW55IHN0cnVjdHVyZSB0aHJvdWdoIHRoZSBAcHJpdiBhcmd1bWVudD8gVGhh
dCB3b3VsZA0KbWFrZSB0aGlzIGZ1bmN0aW9uIG1vcmUgZ2VuZXJhbCBhbmQgbWF5YmUgYWxzbyBt
b3JlIHVzZWZ1bCB0byBvdGhlciBibG9jaw0KZHJpdmVycy4NCg0KQmFydC4=

[GIT PULL] nvme update for Linux 4.14, take 2

[Bart Van Assche]

On Wed, 2017-08-30@19:47 +0300, Sagi Grimberg wrote:
> > > If we get to choose, my preference would be to restore the old behavior
> > > because currently existing nvme transports keep their internal ctrl
> > > representation in the tagset->driver_data as they implement
> > > init/exit_request. Bouncing in nvme core would require to change that
> > > and always keep struct nvme_ctrl as the tagset->driver_data and convert
> > > it on every handler...
> > > 
> > > Everything is doable but it seems like an unneeded hassle to me...
> > 
> > Sorry but I'm not convinced that it would be necessary to change what
> > tagset->driver_data points at. How about moving blk_mq_reinit_tagset() from
> > the block layer core to the NVMe core, to rename it and to pass a pointer
> > to the nvme_ctrl data structure to that function instead of only block layer
> > information?
> 
> That would mean that I need to open-code the tagset iteration in nvme
> which does not feel like something a driver should do.

How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
to make the argument list of blk_mq_tagset_iter() more similar to that of
blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
can pass a pointer to any structure through the @priv argument? That would
make this function more general and maybe also more useful to other block
drivers.

Bart.

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> That would mean that I need to open-code the tagset iteration in nvme
>> which does not feel like something a driver should do.
> 
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

We could do that. But it feels like trying to go head over heals just to
keep a change titled:

"blk-mq: Make blk_mq_reinit_tagset() calls easier to read"

Which I'm not exactly sure I share the motivation. Also, I kinda liked
the symmetry of init/exit/reinit_request calling convention.

But, if you absolutely think its necessary to keep the change, we can
add an "all tags" iterator and use that to implement reinit_request in
nvme.

Christoph? Jens? verdict before we go forward here?

[GIT PULL] nvme update for Linux 4.14, take 2

[Sagi Grimberg]

>> That would mean that I need to open-code the tagset iteration in nvme
>> which does not feel like something a driver should do.
> 
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

We could do that. But it feels like trying to go head over heals just to
keep a change titled:

"blk-mq: Make blk_mq_reinit_tagset() calls easier to read"

Which I'm not exactly sure I share the motivation. Also, I kinda liked
the symmetry of init/exit/reinit_request calling convention.

But, if you absolutely think its necessary to keep the change, we can
add an "all tags" iterator and use that to implement reinit_request in
nvme.

Christoph? Jens? verdict before we go forward here?

Re: [GIT PULL] nvme update for Linux 4.14, take 2

[hch]

On Wed, Aug 30, 2017 at 04:56:28PM +0000, Bart Van Assche wrote:
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

I like that idea.  Even if it makes Sagi's work slightly harder :)

Can we get that into 4.14 still to have a good base?

[GIT PULL] nvme update for Linux 4.14, take 2

[hch]

On Wed, Aug 30, 2017@04:56:28PM +0000, Bart Van Assche wrote:
> How about renaming blk_mq_reinit_tagset() into blk_mq_tagset_iter() and
> to make the argument list of blk_mq_tagset_iter() more similar to that of
> blk_mq_queue_tag_busy_iter() such that callers of blk_mq_tagset_iter()
> can pass a pointer to any structure through the @priv argument? That would
> make this function more general and maybe also more useful to other block
> drivers.

I like that idea.  Even if it makes Sagi's work slightly harder :)

Can we get that into 4.14 still to have a good base?