IPoIB fixes for 4.13
The following changes since commit a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:
RDMA/core: Initialize port_num in qp_attr (2017-07-20 11:24:13 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git tags/rdma-rc-2017-07-26
for you to fetch changes up to 5dc78ad1904db597bdb4427f3ead437aae86f54c:
IB/ipoib: Notify on modify QP failure only when relevant (2017-07-23 10:52:00 +0300)
----------------------------------------------------------------
IPoIB fixes for 4.13
The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
There is one exception in this IPoIB group - addition of patch revert:
Revert "IB/core: Allow QP state transition from reset to error", but it followed by proper fix to the annoying print, so I thought it is appropriate to include it.
Thanks
----------------------------------------------------------------
Alex Vesker (2):
IB/ipoib: Prevent setting negative values to max_nonsrq_conn_qp IB/ipoib: Add multicast packets statistics
Erez Shitrit (3):
IB/ipoib: Use cancel_delayed_work_sync when needed IB/ipoib: Make sure no in-flight joins while leaving that mcast IB/ipoib: Notify on modify QP failure only when relevant
Feras Daoud (3):
IB/ipoib: Fix race between light events and interface restart IB/ipoib: Set IPOIB_NEIGH_TBL_FLUSH after flushed completion initialization IB/ipoib: Add get statistics support to SRIOV VF
Leon Romanovsky (3):
IB/ipoib: Clean error paths in add port
IB/ipoib: Remove double pointer assigning
Revert "IB/core: Allow QP state transition from reset to error"
drivers/infiniband/core/verbs.c | 1 - drivers/infiniband/ulp/ipoib/ipoib.h | 1 + drivers/infiniband/ulp/ipoib/ipoib_cm.c | 1 - drivers/infiniband/ulp/ipoib/ipoib_ethtool.c | 3 ++- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 25 ++++++++++++++++++- drivers/infiniband/ulp/ipoib/ipoib_main.c | 19 +++++++++------ drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 33 +++++++++----------------- 7 files changed, 50 insertions(+), 33 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20170726181232.7363-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
PATCH 1/11 IB/ipoib: Fix race between light events and interface restart
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
A potential race between light_event and interface restart may attach multicast group to an already attached QP.
Scenario:light_event flow goes through ipoib_mcast_dev_flush function, if a context switch occurs before calling ipoib_mcast_remove_list, then we may face a situation where the broadcast of the priv is null and the corresponding QP is not detached yet.
If an "interface restart" runs during the previous context switch, the following scenario occurs:
When the device goes up, ipoib_ib_dev_up function will be called, it will send a new registration request to the broadcast group and then attach the group to the QP that was not detached before.
IPOIB_FLUSH_LIGHT INTERFACE RESTART
__ipoib_ib_dev_flush |
| |
| |
| |
ipoib_mcast_dev_flush |
Move mcast list and broadcast to remove_list |
| |
| |
Context Switch--> |
| ipoib_ib_dev_down
| |
| |
| ipoib_ib_dev_up
| |
| |
| ipoib_mcast_join_task
| allocate new broadcast
| |
| |
| Attach QP to multicast group
| |
| |
| <--Context Switch
ipoib_mcast_leave
Detach QP from multicast group
--- drivers/infiniband/ulp/ipoib/ipoib.h | 1 + drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 + drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 2 ++ 3 files changed, 4 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib.h +1 -0
@@ -336,6 +336,7 @@ struct ipoib_dev_priv {unsigned long flags;struct rw_semaphore vlan_rwsem; +struct mutex mcast_mutex;struct rb_root path_tree;struct list_head path_list;
drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -0
@@ -1877,6 +1877,7 @@ static void ipoib_build_priv(struct net_device *dev) priv->dev = dev;spin_lock_init (&priv->lock);init_rwsem (&priv->vlan_rwsem); +mutex_init (&priv->mcast_mutex);INIT_LIST_HEAD (&priv->path_list);INIT_LIST_HEAD (&priv->child_intfs);
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +2 -0
@@ -838,6 +838,7 @@ void ipoib_mcast_dev_flush(struct net_device *dev)struct ipoib_mcast *mcast, *tmcast;unsigned long flags; +mutex_lock (&priv->mcast_mutex);ipoib_dbg_mcast (priv,"flushing multicast list\n" );spin_lock_irqsave (&priv->lock, flags);
@@ -865,6 +866,7 @@ void ipoib_mcast_dev_flush(struct net_device *dev)wait_for_completion (&mcast->done);ipoib_mcast_remove_list (&remove_list); +mutex_unlock (&priv->mcast_mutex); }static int ipoib_mcast_addr_is_valid (const u8 *addr,const u8 *broadcast)
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/11 IB/ipoib: Use cancel_delayed_work_sync when needed
In reply to a message not in the archiveFrom: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
The work mcast_task can re-queue itself, so instead of doing cancel && flush_workqueue, that still can leave a queued task on the air, use cancel_delayed_work_sync.
Also, no need to use lock over the cancel, the original lock was due to bit assignment setting (IPOIB_MCAST_RUN) that is not in use anymore.
--- drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +1 -6
@@ -684,15 +684,10 @@ void ipoib_mcast_start_thread(struct net_device *dev)int ipoib_mcast_stop_thread (struct net_device *dev) {struct ipoib_dev_priv *priv =ipoib_priv (dev); -unsigned long flags;ipoib_dbg_mcast (priv,"stopping multicast thread\n" ); -spin_lock_irqsave (&priv->lock, flags); -cancel_delayed_work (&priv->mcast_task); -spin_unlock_irqrestore (&priv->lock, flags); - -flush_workqueue (priv->wq); +cancel_delayed_work_sync (&priv->mcast_task);return 0 ; }
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/11 IB/ipoib: Make sure no in-flight joins while leaving that mcast
In reply to a message not in the archiveFrom: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
While cleaning neighs and there is a send-only mcast neigh, the driver should wait to finish its join process before trying to remove it.
Without this patch, we will see messages like: "ipoib_mcast_leave on an in-flight join" and unexpected results in the join_complete.
--- drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_multicast.c +8 -16
@@ -743,6 +743,14 @@ void ipoib_mcast_remove_list(struct list_head *remove_list) {struct ipoib_mcast *mcast, *tmcast; +/* +* make sure the in-flight joins have finished before we attempt +* to leave +*/ +list_for_each_entry_safe (mcast, tmcast, remove_list, list) +if (test_bit (IPOIB_MCAST_FLAG_BUSY , &mcast->flags)) +wait_for_completion (&mcast->done); +list_for_each_entry_safe (mcast, tmcast, remove_list, list) {ipoib_mcast_leave (mcast->dev, mcast);ipoib_mcast_free (mcast);
@@ -852,14 +860,6 @@ void ipoib_mcast_dev_flush(struct net_device *dev)spin_unlock_irqrestore (&priv->lock, flags); -/* -* make sure the in-flight joins have finished before we attempt -* to leave -*/ -list_for_each_entry_safe (mcast, tmcast, &remove_list, list) -if (test_bit (IPOIB_MCAST_FLAG_BUSY , &mcast->flags)) -wait_for_completion (&mcast->done); -ipoib_mcast_remove_list (&remove_list);mutex_unlock (&priv->mcast_mutex); }
@@ -979,14 +979,6 @@ void ipoib_mcast_restart_task(struct work_struct *work)netif_addr_unlock (dev);local_irq_restore (flags); -/* -* make sure the in-flight joins have finished before we attempt -* to leave -*/ -list_for_each_entry_safe (mcast, tmcast, &remove_list, list) -if (test_bit (IPOIB_MCAST_FLAG_BUSY , &mcast->flags)) -wait_for_completion (&mcast->done); -ipoib_mcast_remove_list (&remove_list);/*
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/11 IB/ipoib: Prevent setting negative values to max_nonsrq_conn_qp
In reply to a message not in the archiveFrom: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Don't allow negative values to max_nonsrq_conn_qp. There is no functional impact on a negative value but it is logicically incorrect.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -0
@@ -2366,6 +2366,7 @@ static int __init ipoib_init_module(void) ipoib_sendq_size =max3 (ipoib_sendq_size,2 *MAX_SEND_CQE ,IPOIB_MIN_QUEUE_SIZE );#ifdef CONFIG_INFINIBAND_IPOIB_CM ipoib_max_conn_qp =min (ipoib_max_conn_qp,IPOIB_CM_MAX_CONN_QP ); + ipoib_max_conn_qp =max (ipoib_max_conn_qp,0 );#endif /*
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/11 IB/ipoib: Set IPOIB_NEIGH_TBL_FLUSH after flushed completion initialization
In reply to a message not in the archiveFrom: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Set IPOIB_NEIGH_TBL_FLUSH bit after initializing the neighbor flushed completion, otherwise the garbage collector may signal a completion while it is not initialized yet.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -1
@@ -1560,6 +1560,7 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)int i, wait_flushed =0 ;init_completion (&priv->ntbl.flushed); +set_bit (IPOIB_NEIGH_TBL_FLUSH , &priv->flags);spin_lock_irqsave (&priv->lock, flags);
@@ -1604,7 +1605,6 @@ static void ipoib_neigh_hash_uninit(struct net_device *dev)ipoib_dbg (priv,"ipoib_neigh_hash_uninit\n" );init_completion (&priv->ntbl.deleted); -set_bit (IPOIB_NEIGH_TBL_FLUSH , &priv->flags);/* Stop GC if called at init fail need to cancel work */ stopped =test_and_set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags);
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 6/11 IB/ipoib: Add multicast packets statistics
In reply to a message not in the archiveFrom: Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Update the multicast counter when multicast packets are received and provide this information through ethtool support.
--- drivers/infiniband/ulp/ipoib/ipoib_ethtool.c | 3 ++- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_ethtool.c +2 -1
@@ -52,7 +52,8 @@ static const struct ipoib_stats ipoib_gstrings_stats[] = {IPOIB_NETDEV_STAT (tx_bytes),IPOIB_NETDEV_STAT (tx_errors),IPOIB_NETDEV_STAT (rx_dropped), - IPOIB_NETDEV_STAT(tx_dropped) +IPOIB_NETDEV_STAT (tx_dropped), + IPOIB_NETDEV_STAT(multicast), };#define IPOIB_GLOBAL_STATS_LEN ARRAY_SIZE(ipoib_gstrings_stats)
drivers/infiniband/ulp/ipoib/ipoib_ib.c +2 -0
@@ -256,6 +256,8 @@ static void ipoib_ib_handle_rx_wc(struct net_device *dev, struct ib_wc *wc) ++dev->stats.rx_packets; dev->stats.rx_bytes += skb->len; +if (skb->pkt_type ==PACKET_MULTICAST ) + dev->stats.multicast++; skb->dev = dev; if ((dev->features & NETIF_F_RXCSUM) &&
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 7/11 IB/ipoib: Add get statistics support to SRIOV VF
In reply to a message not in the archiveFrom: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Add SRIOV VF support to get traffic statistics.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -0
@@ -1847,6 +1847,7 @@ static const struct net_device_ops ipoib_netdev_ops_vf = { .ndo_tx_timeout = ipoib_timeout, .ndo_set_rx_mode = ipoib_set_mcast_list, .ndo_get_iflink = ipoib_get_iflink, + .ndo_get_stats64 = ipoib_get_stats, };void ipoib_setup_common (struct net_device *dev)
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 8/11 IB/ipoib: Clean error paths in add port
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Refactor error paths in ipoib_add_port() function. The code flow ensures that the function terminates on every error flow and it makes redundant all "else" cases.
The functions are called during the flow are returning "result < 0", in case of error, so there is no need to check it explicitly.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_main.c +8 -6
@@ -2175,14 +2175,14 @@ static struct net_device *ipoib_add_port(const char *format, priv->dev->dev_id = port -1 ; result =ib_query_port (hca, port, &attr); -if (!result) - priv->max_ib_mtu =ib_mtu_enum_to_int (attr.max_mtu); -else { +if (result) {printk (KERN_WARNING "%s: ib_query_port %d failed\n" , hca->name, port); goto device_init_failed; } + priv->max_ib_mtu =ib_mtu_enum_to_int (attr.max_mtu); +/* MTU will be reset when mcast join happens */ priv->dev->mtu =IPOIB_UD_MTU (priv->max_ib_mtu); priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu;
@@ -2213,12 +2213,14 @@ static struct net_device *ipoib_add_port(const char *format,printk (KERN_WARNING "%s: ib_query_gid port %d failed (ret = %d)\n" , hca->name, port, result); goto device_init_failed; - }else -memcpy (priv->dev->dev_addr +4 , priv->local_gid.raw,sizeof (union ib_gid )); + } + +memcpy (priv->dev->dev_addr +4 , priv->local_gid.raw, +sizeof (union ib_gid ));set_bit (IPOIB_FLAG_DEV_ADDR_SET , &priv->flags); result =ipoib_dev_init (priv->dev, hca, port); -if (result <0 ) { +if (result) {printk (KERN_WARNING "%s: failed to initialize port %d (ret = %d)\n" , hca->name, port, result); goto device_init_failed;
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 9/11 IB/ipoib: Remove double pointer assigning
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
There is no need to assign "p" pointer twice.
This patch fixes the following smatch warning:drivers/infiniband/ulp/ipoib/ipoib_cm.c:517 ipoib_cm_rx_handler() warn:
missing break? reassigning 'p->id'
--- drivers/infiniband/ulp/ipoib/ipoib_cm.c | 1 - 1 file changed, 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -1
@@ -511,7 +511,6 @@ static int ipoib_cm_rx_handler(struct ib_cm_id *cm_id,case IB_CM_REQ_RECEIVED :return ipoib_cm_req_handler (cm_id, event);case IB_CM_DREQ_RECEIVED : - p = cm_id->context;ib_send_cm_drep (cm_id,NULL ,0 );/* Fall through */ case IB_CM_REJ_RECEIVED :
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 10/11 Revert "IB/core: Allow QP state transition from reset to error"
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The commit ebc9ca43e1d5 ("IB/core: Allow QP state transition from reset to error") allowed transition from Reset to Error state for the QPs. This behavior doesn't follow the IBTA specification 1.3, which in 10.3.1 QUEUE PAIR AND EE CONTEXT STATES section.
The quote from the spec:
"An error can be forced from any state, except Reset, with the Modify QP/EE Verb."
--- drivers/infiniband/core/verbs.c | 1 - 1 file changed, 1 deletion(-)
drivers/infiniband/core/verbs.c +0 -1
@@ -895,7 +895,6 @@ static const struct { } qp_state_table[IB_QPS_ERR + 1][IB_QPS_ERR + 1] = { [IB_QPS_RESET] = { [IB_QPS_RESET] = { .valid = 1 }, - [IB_QPS_ERR] = { .valid = 1 }, [IB_QPS_INIT] = { .valid = 1, .req_param = {
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20170726181232.7363-11-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Re: [rdma-rc 10/11] Revert "IB/core: Allow QP state transition from reset to error"
Quoted 23 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The commit
ebc9ca43e1d5("IB/core: Allow QP state transition from reset to error") allowed transition from Reset to Error state for the QPs. This behavior doesn't follow the IBTA specification 1.3, which in 10.3.1 QUEUE PAIR AND EE CONTEXT STATES section.The quote from the spec:
"An error can be forced from any state, except Reset, with the Modify QP/EE Verb."Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/verbs.c | 1 - 1 file changed, 1 deletion(-)drivers/infiniband/core/verbs.c +0
-0
drivers/infiniband/core/verbs.c static const struct { · line 900
Quoted 7 lines
drivers/infiniband/core/verbs.c +0
-1@@ -895,7 +895,6 @@ static const struct { } qp_state_table[IB_QPS_ERR + 1][IB_QPS_ERR + 1] = { [IB_QPS_RESET] = { [IB_QPS_RESET] = { .valid = 1 }, - [IB_QPS_ERR] = { .valid = 1 }, [IB_QPS_INIT] = { .valid = 1, .req_param = {
The diagram from that section clearly shows you can't go from reset to error. However table 91 as in our original commit message says it could go to error from any state. Taking a step back and thinking about it, I'm not sure it really makes sense to be able to go from reset to error anyway. There is nothing that can go wrong until the qp is transitioned out of reset really.
Any idea why this patch was never adopted:
http://www.spinics.net/lists/linux-rdma/msg07627.html
This was during my storage hiatus so I wasn't following things RDMA back then. However this seems like the correct approach to me. Granted that patch would look different as the code has moved around but the same sort of check could go in ipoib_ib_dev_stop_default().
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 49fe625b-cd44-137e-c001-1d85297f4e62-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Dennis Dalessandro
Reviewed-byRe: [rdma-rc 10/11] Revert "IB/core: Allow QP state transition from reset to error"
Quoted 34 lines · Dennis Dalessandro
Quoted 23 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The commit
ebc9ca43e1d5("IB/core: Allow QP state transition from reset to error") allowed transition from Reset to Error state for the QPs. This behavior doesn't follow the IBTA specification 1.3, which in 10.3.1 QUEUE PAIR AND EE CONTEXT STATES section.The quote from the spec:
"An error can be forced from any state, except Reset, with the Modify QP/EE Verb."Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/verbs.c | 1 - 1 file changed, 1 deletion(-)drivers/infiniband/core/verbs.c +0
-0drivers/infiniband/core/verbs.c static const struct { · line 900
Quoted 7 lines
drivers/infiniband/core/verbs.c +0
-1@@ -895,7 +895,6 @@ static const struct { } qp_state_table[IB_QPS_ERR + 1][IB_QPS_ERR + 1] = { [IB_QPS_RESET] = { [IB_QPS_RESET] = { .valid = 1 }, - [IB_QPS_ERR] = { .valid = 1 }, [IB_QPS_INIT] = { .valid = 1, .req_param = {The diagram from that section clearly shows you can't go from reset to error. However table 91 as in our original commit message says it could go to error from any state. Taking a step back and thinking about it, I'm not sure it really makes sense to be able to go from reset to error anyway. There is nothing that can go wrong until the qp is transitioned out of reset really.
Quoted 6 lines
Any idea why this patch was never adopted:
http://www.spinics.net/lists/linux-rdma/msg07627.htmlThis was during my storage hiatus so I wasn't following things RDMA back then. However this seems like the correct approach to me. Granted that patch would look different as the code has moved around but the same sort of check could go in
ipoib_ib_dev_stop_default().-Denny
Ah guess I should have waited until reading the next patch 11/11!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): fcacf34e-d16f-3d66-2877-be5b5993f296-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Re: [rdma-rc 10/11] Revert "IB/core: Allow QP state transition from reset to error"
Quoted 50 lines · Dennis Dalessandro
Quoted 49 lines · Dennis Dalessandro
Quoted 25 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The commit
ebc9ca43e1d5("IB/core: Allow QP state transition from reset to error")
allowed transition from Reset to Error state for the QPs. This behavior doesn't follow the IBTA specification 1.3, which in 10.3.1 QUEUE PAIR AND EE CONTEXT STATES section.The quote from the spec:
"An error can be forced from any state, except Reset, with the Modify QP/EE Verb."Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/verbs.c | 1 - 1 file changed, 1 deletion(-)drivers/infiniband/core/verbs.c +0
-0drivers/infiniband/core/verbs.c static const struct { · line 900
Quoted 7 lines
drivers/infiniband/core/verbs.c +0
-1@@ -895,7 +895,6 @@ static const struct { } qp_state_table[IB_QPS_ERR + 1][IB_QPS_ERR + 1] = { [IB_QPS_RESET] = { [IB_QPS_RESET] = { .valid = 1 }, - [IB_QPS_ERR] = { .valid = 1 }, [IB_QPS_INIT] = { .valid = 1, .req_param = {The diagram from that section clearly shows you can't go from reset to error. However table 91 as in our original commit message says it could go to error from any state. Taking a step back and thinking about it, I'm not sure it really makes sense to be able to go from reset to error anyway. There is nothing that can go wrong until the qp is transitioned out of reset really.
Any idea why this patch was never adopted:
http://www.spinics.net/lists/linux-rdma/msg07627.htmlThis was during my storage hiatus so I wasn't following things RDMA back then. However this seems like the correct approach to me. Granted that patch would look different as the code has moved around but the same sort of check could go in
ipoib_ib_dev_stop_default().
Quoted 5 lines
Quoted 1 line
-Denny
Ah guess I should have waited until reading the next patch 11/11!
Reviewed-by Dennis Dalessandro <[email protected]>
Thanks Dennis,
I agree with you, the spec is really misleading.
PATCH 11/11 IB/ipoib: Notify on modify QP failure only when relevant
In reply to a message not in the archiveFrom: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Modify QP can fail and it can be acceptable, like when moving from RST to ERR state, all the rest are not acceptable and a message to the log should be printed.
The current code prints on all failures and many messages like:
"Failed to modify QP to ERROR state" appear, even when supported by the state machine of the QP object.
--- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_ib.c +22 -1
@@ -711,6 +711,27 @@ static int recvs_pending(struct net_device *dev)return pending; } +static void check_qp_movement_and_print (struct ipoib_dev_priv *priv, +struct ib_qp *qp, +enum ib_qp_state new_state) +{ +struct ib_qp_attr qp_attr; +struct ib_qp_init_attr query_init_attr; +int ret; + + ret =ib_query_qp (qp, &qp_attr,IB_QP_STATE , &query_init_attr); +if (ret) { +ipoib_warn (priv,"%s: Failed to query QP\n" , __func__); +return ; + } +/* print according to the new-state and the previous state.*/ +if (new_state ==IB_QPS_ERR && qp_attr.qp_state ==IB_QPS_RESET ) +ipoib_dbg (priv,"Failed modify QP, IB_QPS_RESET to IB_QPS_ERR, acceptable\n" ); +else +ipoib_warn (priv,"Failed to modify QP to state: %d from state: %d\n" , + new_state, qp_attr.qp_state); +} +int ipoib_ib_dev_stop_default (struct net_device *dev) {struct ipoib_dev_priv *priv =ipoib_priv (dev);
@@ -730,7 +751,7 @@ int ipoib_ib_dev_stop_default(struct net_device *dev)*/ qp_attr.qp_state =IB_QPS_ERR ;if (ib_modify_qp (priv->qp, &qp_attr,IB_QP_STATE )) -ipoib_warn (priv,"Failed to modify QP to ERROR state\n" ); +check_qp_movement_and_print (priv, priv->qp,IB_QPS_ERR );/* Wait for all sends and receives to complete */ begin = jiffies;
Signature · 6 lines
2.13.3 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20170726181232.7363-12-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Dennis Dalessandro
Reviewed-byRe: [rdma-rc 11/11] IB/ipoib: Notify on modify QP failure only when relevant
Quoted 34 lines · Leon Romanovsky
From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Modify QP can fail and it can be acceptable, like when moving from RST to ERR state, all the rest are not acceptable and a message to the log should be printed.
The current code prints on all failures and many messages like:
"Failed to modify QP to ERROR state" appear, even when supported by the state machine of the QP object.Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)drivers/infiniband/ulp/ipoib/ipoib_ib.c +10
-0@@ -711,6 +711,27 @@ static int recvs_pending(struct net_device *dev)return pending; } +static void check_qp_movement_and_print (struct ipoib_dev_priv *priv, +struct ib_qp *qp, +enum ib_qp_state new_state) +{ +struct ib_qp_attr qp_attr; +struct ib_qp_init_attr query_init_attr; +int ret; + + ret =ib_query_qp (qp, &qp_attr,IB_QP_STATE , &query_init_attr); +if (ret) {
drivers/infiniband/ulp/ipoib/ipoib_ib.c static int recvs_pending(struct net_device *dev) · line 733
Quoted 10 lines
drivers/infiniband/ulp/ipoib/ipoib_ib.c +10
-0+ipoib_warn (priv,"%s: Failed to query QP\n" , __func__); +return ; + } +/* print according to the new-state and the previous state.*/ +if (new_state ==IB_QPS_ERR && qp_attr.qp_state ==IB_QPS_RESET ) +ipoib_dbg (priv,"Failed modify QP, IB_QPS_RESET to IB_QPS_ERR, acceptable\n" ); +else +ipoib_warn (priv,"Failed to modify QP to state: %d from state: %d\n" , + new_state, qp_attr.qp_state); +}
So debug message instead of warn. I can live with that.
Quoted 6 lines
drivers/infiniband/ulp/ipoib/ipoib_ib.c +1
-0+int ipoib_ib_dev_stop_default (struct net_device *dev) {struct ipoib_dev_priv *priv =ipoib_priv (dev);
drivers/infiniband/ulp/ipoib/ipoib_ib.c int ipoib_ib_dev_stop_default(struct net_device *dev) · line 757
Quoted 8 lines
drivers/infiniband/ulp/ipoib/ipoib_ib.c +1
-1@@ -730,7 +751,7 @@ int ipoib_ib_dev_stop_default(struct net_device *dev)*/ qp_attr.qp_state =IB_QPS_ERR ;if (ib_modify_qp (priv->qp, &qp_attr,IB_QP_STATE )) -ipoib_warn (priv,"Failed to modify QP to ERROR state\n" ); +check_qp_movement_and_print (priv, priv->qp,IB_QPS_ERR );/* Wait for all sends and receives to complete */ begin = jiffies;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 12 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize port_num in qp_attr (2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git tags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant (2017-07-23 10:52:00 +0300)
Quoted 8 lines
----------------------------------------------------------------
IPoIB fixes for 4.13The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
I'm not sure that patches 4,6,7,8, and 11 really should go in the RC. I think those should go to for-next. My opinion anyway.
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 2679977c-6258-ed15-a234-3682b8be65f3-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Quoted 18 lines · Dennis Dalessandro
Quoted 17 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize port_num in qp_attr (2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
tags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant
(2017-07-23 10:52:00 +0300)----------------------------------------------------------------
IPoIB fixes for 4.13
Quoted 7 lines
Quoted 5 lines
The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
I'm not sure that patches 4,6,7,8, and 11 really should go in the RC. I think those should go to for-next. My opinion anyway.
Ugh, one of those days, hands move faster than brain. That should have said 9 instead of 11.
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 3eb7d0e3-b1ef-533e-56ae-9cc6b65ddfc5-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
Quoted 24 lines · Dennis Dalessandro
Quoted 23 lines · Dennis Dalessandro
Quoted 22 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize port_num in qp_attr (2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
tags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant
(2017-07-23 10:52:00 +0300)----------------------------------------------------------------
IPoIB fixes for 4.13The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Most of the patches are from pre-git era and hence lack of Fixes lines.
I'm not sure that patches 4,6,7,8, and 11 really should go in the RC. I think those should go to for-next. My opinion anyway.
Ugh, one of those days, hands move faster than brain. That should have said 9 instead of 11.
They are one-liners, simple fixes, all in one place and this is why I grouped them together.
Quoted 3 lines
-Denny
Quoted 26 lines · Dennis Dalessandro
Quoted 25 lines · Dennis Dalessandro
Quoted 23 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize port_num in qp_attr (2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
tags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant
(2017-07-23 10:52:00 +0300)----------------------------------------------------------------
IPoIB fixes for 4.13The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
Quoted 8 lines
Quoted 2 lines
I'm not sure that patches 4,6,7,8, and 11 really should go in the RC. I think those should go to for-next. My opinion anyway.
Ugh, one of those days, hands move faster than brain. That should have said 9 instead of 11.
-Denny
I agree with Denny on 6 and 7, but I think the others are reasonable fixes. And given that both 6 and 7 are so small, I'm OK letting them pass through this time. I've pulled this series and pushed the resulting k.o/for-4.13-rc branch to github. Once it clears 0day testing I'll push to k.o and send a pull request since we don't have any other -rc submissions queued at the moment that I know of.
Signature · 3 lines
Doug Ledford <[email protected]> GPG Key ID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Quoted 47 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize
port_numinqp_attr(2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.gittags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant (2017-07-23 10:52:00 +0300)
----------------------------------------------------------------
IPoIB fixes for 4.13The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
There is one exception in this IPoIB group - addition of patch revert:
Revert "IB/core: Allow QP state transition from reset to error", but it followed by proper fix to the annoying print, so I thought it is appropriate to include it.Thanks
----------------------------------------------------------------
Alex Vesker (2):
IB/ipoib: Prevent setting negative values tomax_nonsrq_conn_qpIB/ipoib: Add multicast packets statisticsErez Shitrit (3):
IB/ipoib: Usecancel_delayed_work_syncwhen needed IB/ipoib: Make sure no in-flight joins while leaving that mcast IB/ipoib: Notify on modify QP failure only when relevantFeras Daoud (3):
IB/ipoib: Fix race between light events and interface restart IB/ipoib: SetIPOIB_NEIGH_TBL_FLUSHafter flushed completion initialization IB/ipoib: Add get statistics support to SRIOV VFLeon Romanovsky (3):
IB/ipoib: Clean error paths in add port
IB/ipoib: Remove double pointer assigning
Revert "IB/core: Allow QP state transition from reset to error"
Quoted 8 lines
drivers/infiniband/core/verbs.c | 1 - drivers/infiniband/ulp/ipoib/ipoib.h | 1 + drivers/infiniband/ulp/ipoib/ipoib_cm.c | 1 - drivers/infiniband/ulp/ipoib/ipoib_ethtool.c | 3 ++- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 25 ++++++++++++++++++- drivers/infiniband/ulp/ipoib/ipoib_main.c | 19 +++++++++------ drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 33 +++++++++----------------- 7 files changed, 50 insertions(+), 33 deletions(-)
Reminder
A message that is not in the archive (sent off-list or not yet received): 20170806082018.GD3636-U/[email protected]
Quoted 58 lines · Leon Romanovsky
Quoted 47 lines · Leon Romanovsky
The following changes since commit
a62ab66b13a0f9bcb17b7b761f6670941ed5cd62:RDMA/core: Initialize
port_numinqp_attr(2017-07-20 11:24:13 -0400)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.gittags/rdma-rc-2017-07-26for you to fetch changes up to
5dc78ad1904db597bdb4427f3ead437aae86f54c:IB/ipoib: Notify on modify QP failure only when relevant (2017-07-23 10:52:00 +0300)
----------------------------------------------------------------
IPoIB fixes for 4.13The patchset provides various fixes for IPoIB. It is combination of fixes to various issues discovered during verification along with static checkers cleanup patches.
Most of the patches are from pre-git era and hence lack of Fixes lines.
There is one exception in this IPoIB group - addition of patch revert:
Revert "IB/core: Allow QP state transition from reset to error", but it followed by proper fix to the annoying print, so I thought it is appropriate to include it.Thanks
----------------------------------------------------------------
Alex Vesker (2):
IB/ipoib: Prevent setting negative values tomax_nonsrq_conn_qpIB/ipoib: Add multicast packets statisticsErez Shitrit (3):
IB/ipoib: Usecancel_delayed_work_syncwhen needed IB/ipoib: Make sure no in-flight joins while leaving that mcast IB/ipoib: Notify on modify QP failure only when relevantFeras Daoud (3):
IB/ipoib: Fix race between light events and interface restart IB/ipoib: SetIPOIB_NEIGH_TBL_FLUSHafter flushed completion initialization IB/ipoib: Add get statistics support to SRIOV VFLeon Romanovsky (3):
IB/ipoib: Clean error paths in add port
IB/ipoib: Remove double pointer assigning
Revert "IB/core: Allow QP state transition from reset to error"Quoted 8 lines
drivers/infiniband/core/verbs.c | 1 - drivers/infiniband/ulp/ipoib/ipoib.h | 1 + drivers/infiniband/ulp/ipoib/ipoib_cm.c | 1 - drivers/infiniband/ulp/ipoib/ipoib_ethtool.c | 3 ++- drivers/infiniband/ulp/ipoib/ipoib_ib.c | 25 ++++++++++++++++++- drivers/infiniband/ulp/ipoib/ipoib_main.c | 19 +++++++++------ drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 33 +++++++++----------------- 7 files changed, 50 insertions(+), 33 deletions(-)
Quoted 1 line
Reminder
Yep, was planning on going through these today.
Signature · 3 lines
Doug Ledford <[email protected]> GPG Key ID: B826A3330E572FDD Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD