| Message ID | 20230712161513.134860-11-aaptel@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | nvme-tcp receive offloads | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On 7/12/23 19:14, Aurelien Aptel wrote: > From: Or Gerlitz <ogerlitz@nvidia.com> > > For ddp setup/teardown and resync, the offloading logic > uses HW resources at the NIC driver such as SQ and CQ. > > These resources are destroyed when the netdevice does down > and hence we must stop using them before the NIC driver > destroys them. > > Use netdevice notifier for that matter -- offloaded connections > are stopped before the stack continues to call the NIC driver > close ndo. > > We use the existing recovery flow which has the advantage > of resuming the offload once the connection is re-set. > > This also buys us proper handling for the UNREGISTER event > b/c our offloading starts in the UP state, and down is always > there between up to unregister. > > Signed-off-by: Or Gerlitz <ogerlitz@nvidia.com> > Signed-off-by: Boris Pismenny <borisp@nvidia.com> > Signed-off-by: Ben Ben-Ishay <benishay@nvidia.com> > Signed-off-by: Yoray Zack <yorayz@nvidia.com> > Signed-off-by: Shai Malin <smalin@nvidia.com> > Signed-off-by: Aurelien Aptel <aaptel@nvidia.com> > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/nvme/host/tcp.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c > index df58668cbad6..e68e5da3df76 100644 > --- a/drivers/nvme/host/tcp.c > +++ b/drivers/nvme/host/tcp.c > @@ -221,6 +221,7 @@ struct nvme_tcp_ctrl { > > static LIST_HEAD(nvme_tcp_ctrl_list); > static DEFINE_MUTEX(nvme_tcp_ctrl_mutex); > +static struct notifier_block nvme_tcp_netdevice_nb; > static struct workqueue_struct *nvme_tcp_wq; > static const struct blk_mq_ops nvme_tcp_mq_ops; > static const struct blk_mq_ops nvme_tcp_admin_mq_ops; > @@ -3234,6 +3235,30 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev, > return ERR_PTR(ret); > } > > +static int nvme_tcp_netdev_event(struct notifier_block *this, > + unsigned long event, void *ptr) > +{ > + struct net_device *ndev = netdev_notifier_info_to_dev(ptr); > + struct nvme_tcp_ctrl *ctrl; > + > + switch (event) { > + case NETDEV_GOING_DOWN: > + mutex_lock(&nvme_tcp_ctrl_mutex); > + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { > + if (ndev == ctrl->offloading_netdev) > + nvme_tcp_error_recovery(&ctrl->ctrl); > + } > + mutex_unlock(&nvme_tcp_ctrl_mutex); > + flush_workqueue(nvme_reset_wq); In what context is this called? because every time we flush a workqueue, lockdep finds another reason to complain about something... Otherwise looks good, Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Sagi Grimberg <sagi@grimberg.me> writes: >> + switch (event) { >> + case NETDEV_GOING_DOWN: >> + mutex_lock(&nvme_tcp_ctrl_mutex); >> + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { >> + if (ndev == ctrl->offloading_netdev) >> + nvme_tcp_error_recovery(&ctrl->ctrl); >> + } >> + mutex_unlock(&nvme_tcp_ctrl_mutex); >> + flush_workqueue(nvme_reset_wq); > > In what context is this called? because every time we flush a workqueue, > lockdep finds another reason to complain about something... Thanks for highlighting this, we re-checked it and we found that we are covered by nvme_tcp_error_recovery(), we can remove the flush_workqueue() call above. > Otherwise looks good, > Reviewed-by: Sagi Grimberg <sagi@grimberg.me> Thanks
>>> + switch (event) { >>> + case NETDEV_GOING_DOWN: >>> + mutex_lock(&nvme_tcp_ctrl_mutex); >>> + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { >>> + if (ndev == ctrl->offloading_netdev) >>> + nvme_tcp_error_recovery(&ctrl->ctrl); >>> + } >>> + mutex_unlock(&nvme_tcp_ctrl_mutex); >>> + flush_workqueue(nvme_reset_wq); >> >> In what context is this called? because every time we flush a workqueue, >> lockdep finds another reason to complain about something... > > Thanks for highlighting this, we re-checked it and we found that we are > covered by nvme_tcp_error_recovery(), we can remove the > flush_workqueue() call above. Don't you need to flush at least err_work? How do you know that it completed and put all the references?
Sagi Grimberg <sagi@grimberg.me> writes: >>>> + switch (event) { >>>> + case NETDEV_GOING_DOWN: >>>> + mutex_lock(&nvme_tcp_ctrl_mutex); >>>> + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { >>>> + if (ndev == ctrl->offloading_netdev) >>>> + nvme_tcp_error_recovery(&ctrl->ctrl); >>>> + } >>>> + mutex_unlock(&nvme_tcp_ctrl_mutex); >>>> + flush_workqueue(nvme_reset_wq); >>> >>> In what context is this called? because every time we flush a workqueue, >>> lockdep finds another reason to complain about something... >> >> Thanks for highlighting this, we re-checked it and we found that we are >> covered by nvme_tcp_error_recovery(), we can remove the >> flush_workqueue() call above. > > Don't you need to flush at least err_work? How do you know that it > completed and put all the references? Our bad, we do need to wait for the netdev reference to be put, and we must keep the flush_workqueue(). We did test with lockdep but did not notice any warnings. As for the context of the event handler when you set the link down is the process issuing the netlink syscall. So if you run "ip link set X down" it would be (simplified): "ip" -> syscall -> netlink api -> ... -> do_setlink -> call_netdevice_notifiers_info.
>>>>> + switch (event) { >>>>> + case NETDEV_GOING_DOWN: >>>>> + mutex_lock(&nvme_tcp_ctrl_mutex); >>>>> + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { >>>>> + if (ndev == ctrl->offloading_netdev) >>>>> + nvme_tcp_error_recovery(&ctrl->ctrl); >>>>> + } >>>>> + mutex_unlock(&nvme_tcp_ctrl_mutex); >>>>> + flush_workqueue(nvme_reset_wq); >>>> >>>> In what context is this called? because every time we flush a workqueue, >>>> lockdep finds another reason to complain about something... >>> >>> Thanks for highlighting this, we re-checked it and we found that we are >>> covered by nvme_tcp_error_recovery(), we can remove the >>> flush_workqueue() call above. >> >> Don't you need to flush at least err_work? How do you know that it >> completed and put all the references? > > Our bad, we do need to wait for the netdev reference to be put, and we > must keep the flush_workqueue(). > > We did test with lockdep but did not notice any warnings. I'm assuming you are running with lockdep and friends? > > As for the context of the event handler when you set the link down is > the process issuing the netlink syscall. > > So if you run "ip link set X down" it would be (simplified): > > "ip" -> syscall -> netlink api -> ... -> do_setlink -> call_netdevice_notifiers_info. ok, that should be fine I think.
Sagi Grimberg <sagi@grimberg.me> writes:
> I'm assuming you are running with lockdep and friends?
We tested it under the following configuration:
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_PROVE_LOCKING=y
CONFIG_PROVE_RAW_LOCK_NESTING=y
CONFIG_LOCKDEP=y
CONFIG_LOCKDEP_BITS=15
CONFIG_LOCKDEP_CHAINS_BITS=16
CONFIG_LOCKDEP_STACK_TRACE_BITS=19
CONFIG_LOCKDEP_STACK_TRACE_HASH_BITS=14
CONFIG_LOCKDEP_CIRCULAR_QUEUE_BITS=12
CONFIG_DEBUG_LOCKDEP=y
CONFIG_PROVE_RCU=y
No warnings were printed in dmesg.
Anything else we should verify?
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index df58668cbad6..e68e5da3df76 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -221,6 +221,7 @@ struct nvme_tcp_ctrl { static LIST_HEAD(nvme_tcp_ctrl_list); static DEFINE_MUTEX(nvme_tcp_ctrl_mutex); +static struct notifier_block nvme_tcp_netdevice_nb; static struct workqueue_struct *nvme_tcp_wq; static const struct blk_mq_ops nvme_tcp_mq_ops; static const struct blk_mq_ops nvme_tcp_admin_mq_ops; @@ -3234,6 +3235,30 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev, return ERR_PTR(ret); } +static int nvme_tcp_netdev_event(struct notifier_block *this, + unsigned long event, void *ptr) +{ + struct net_device *ndev = netdev_notifier_info_to_dev(ptr); + struct nvme_tcp_ctrl *ctrl; + + switch (event) { + case NETDEV_GOING_DOWN: + mutex_lock(&nvme_tcp_ctrl_mutex); + list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list) { + if (ndev == ctrl->offloading_netdev) + nvme_tcp_error_recovery(&ctrl->ctrl); + } + mutex_unlock(&nvme_tcp_ctrl_mutex); + flush_workqueue(nvme_reset_wq); + /* + * The associated controllers teardown has completed, + * ddp contexts were also torn down so we should be + * safe to continue... + */ + } + return NOTIFY_DONE; +} + static struct nvmf_transport_ops nvme_tcp_transport = { .name = "tcp", .module = THIS_MODULE, @@ -3248,6 +3273,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = { static int __init nvme_tcp_init_module(void) { + int ret; + BUILD_BUG_ON(sizeof(struct nvme_tcp_hdr) != 8); BUILD_BUG_ON(sizeof(struct nvme_tcp_cmd_pdu) != 72); BUILD_BUG_ON(sizeof(struct nvme_tcp_data_pdu) != 24); @@ -3262,8 +3289,19 @@ static int __init nvme_tcp_init_module(void) if (!nvme_tcp_wq) return -ENOMEM; + nvme_tcp_netdevice_nb.notifier_call = nvme_tcp_netdev_event; + ret = register_netdevice_notifier(&nvme_tcp_netdevice_nb); + if (ret) { + pr_err("failed to register netdev notifier\n"); + goto out_free_workqueue; + } + nvmf_register_transport(&nvme_tcp_transport); return 0; + +out_free_workqueue: + destroy_workqueue(nvme_tcp_wq); + return ret; } static void __exit nvme_tcp_cleanup_module(void) @@ -3271,6 +3309,7 @@ static void __exit nvme_tcp_cleanup_module(void) struct nvme_tcp_ctrl *ctrl; nvmf_unregister_transport(&nvme_tcp_transport); + unregister_netdevice_notifier(&nvme_tcp_netdevice_nb); mutex_lock(&nvme_tcp_ctrl_mutex); list_for_each_entry(ctrl, &nvme_tcp_ctrl_list, list)