diff mbox series

[v2,1/2] RDMA/rxe: Distinguish between down links and disabled links

Message ID 20181101131846.7441-2-andrew.boyer@dell.com (mailing list archive)
State Accepted
Headers show
Series RDMA/rxe: link state and statistics | expand

Commit Message

Andrew Boyer Nov. 1, 2018, 1:18 p.m. UTC 
In ib_query_port(), use the netdev's IFF_UP flag to determine phys_state
(flag set = down = POLLING, flag clear = disabled = DISABLED).

Callers can then use the phys_state field to distinguish between links
which have a dead partner, cable missing, etc., from links which are
turned off on the local node. This is useful for HA and supportability.

Signed-off-by: Andrew Boyer <andrew.boyer@dell.com>
---
 drivers/infiniband/sw/rxe/rxe.h       | 5 +++--
 drivers/infiniband/sw/rxe/rxe_net.c   | 2 --
 drivers/infiniband/sw/rxe/rxe_verbs.c | 8 ++++++++
 3 files changed, 11 insertions(+), 4 deletions(-)

Comments

Zhu Yanjun Nov. 3, 2018, 1:09 p.m. UTC | #1 
On 2018/11/1 21:18, Andrew Boyer wrote:
> In ib_query_port(), use the netdev's IFF_UP flag to determine phys_state
> (flag set = down = POLLING, flag clear = disabled = DISABLED).
>
> Callers can then use the phys_state field to distinguish between links
> which have a dead partner, cable missing, etc., from links which are
> turned off on the local node. This is useful for HA and supportability.
>
> Signed-off-by: Andrew Boyer <andrew.boyer@dell.com>
> ---
>   drivers/infiniband/sw/rxe/rxe.h       | 5 +++--
>   drivers/infiniband/sw/rxe/rxe_net.c   | 2 --
>   drivers/infiniband/sw/rxe/rxe_verbs.c | 8 ++++++++
>   3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index d9ec2de68738..8f79bd86d033 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -65,8 +65,9 @@
>    */
>   #define RXE_UVERBS_ABI_VERSION		2
>   
> -#define IB_PHYS_STATE_LINK_UP		(5)
> -#define IB_PHYS_STATE_LINK_DOWN		(3)
> +#define RDMA_LINK_PHYS_STATE_LINK_UP	(5)
> +#define RDMA_LINK_PHYS_STATE_DISABLED	(3)
> +#define RDMA_LINK_PHYS_STATE_POLLING	(2)
>   
>   #define RXE_ROCE_V2_SPORT		(0xc000)
>   
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 40e82e0f6c2d..cb028a3d8275 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -607,7 +607,6 @@ void rxe_port_up(struct rxe_dev *rxe)
>   
>   	port = &rxe->port;
>   	port->attr.state = IB_PORT_ACTIVE;
> -	port->attr.phys_state = IB_PHYS_STATE_LINK_UP;
>   
>   	rxe_port_event(rxe, IB_EVENT_PORT_ACTIVE);
>   	dev_info(&rxe->ib_dev.dev, "set active\n");
> @@ -620,7 +619,6 @@ void rxe_port_down(struct rxe_dev *rxe)
>   
>   	port = &rxe->port;
>   	port->attr.state = IB_PORT_DOWN;
> -	port->attr.phys_state = IB_PHYS_STATE_LINK_DOWN;
>   
>   	rxe_port_event(rxe, IB_EVENT_PORT_ERR);
>   	dev_info(&rxe->ib_dev.dev, "set down\n");
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 9c19f2027511..4eef87c103b2 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -71,6 +71,14 @@ static int rxe_query_port(struct ib_device *dev,
>   	mutex_lock(&rxe->usdev_lock);
>   	rc = ib_get_eth_speed(dev, port_num, &attr->active_speed,
>   			      &attr->active_width);
> +
> +	if (attr->state == IB_PORT_ACTIVE)
> +		attr->phys_state = RDMA_LINK_PHYS_STATE_LINK_UP;
Sorry. Phy state should the IB device physical state. It should depend 
on the HW.
I am not sure it is a good idea to use other variable to control.

Zhu Yanjun
> +	else if (dev_get_flags(rxe->ndev) & IFF_UP)
> +		attr->phys_state = RDMA_LINK_PHYS_STATE_POLLING;
> +	else
> +		attr->phys_state = RDMA_LINK_PHYS_STATE_DISABLED;
> +
>   	mutex_unlock(&rxe->usdev_lock);
>   
>   out:
Andrew Boyer Nov. 6, 2018, 12:09 p.m. UTC | #2 
On 2018/11/1 21:18, Andrew Boyer wrote:
> In ib_query_port(), use the netdev's IFF_UP flag to determine phys_state
> (flag set = down = POLLING, flag clear = disabled = DISABLED).
>
> Callers can then use the phys_state field to distinguish between links
> which have a dead partner, cable missing, etc., from links which are
> turned off on the local node. This is useful for HA and supportability.
>
> Signed-off-by: Andrew Boyer <andrew.boyer@dell.com>
> ---
>   drivers/infiniband/sw/rxe/rxe.h       | 5 +++--
>   drivers/infiniband/sw/rxe/rxe_net.c   | 2 --
>   drivers/infiniband/sw/rxe/rxe_verbs.c | 8 ++++++++
>   3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
> index d9ec2de68738..8f79bd86d033 100644
> --- a/drivers/infiniband/sw/rxe/rxe.h
> +++ b/drivers/infiniband/sw/rxe/rxe.h
> @@ -65,8 +65,9 @@
>    */
>   #define RXE_UVERBS_ABI_VERSION              2
>
> -#define IB_PHYS_STATE_LINK_UP                (5)
> -#define IB_PHYS_STATE_LINK_DOWN              (3)
> +#define RDMA_LINK_PHYS_STATE_LINK_UP (5)
> +#define RDMA_LINK_PHYS_STATE_DISABLED        (3)
> +#define RDMA_LINK_PHYS_STATE_POLLING (2)
>
>   #define RXE_ROCE_V2_SPORT           (0xc000)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 40e82e0f6c2d..cb028a3d8275 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -607,7 +607,6 @@ void rxe_port_up(struct rxe_dev *rxe)
>
>       port = &rxe->port;
>       port->attr.state = IB_PORT_ACTIVE;
> -     port->attr.phys_state = IB_PHYS_STATE_LINK_UP;
>
>       rxe_port_event(rxe, IB_EVENT_PORT_ACTIVE);
>       dev_info(&rxe->ib_dev.dev, "set active\n");
> @@ -620,7 +619,6 @@ void rxe_port_down(struct rxe_dev *rxe)
>
>       port = &rxe->port;
>       port->attr.state = IB_PORT_DOWN;
> -     port->attr.phys_state = IB_PHYS_STATE_LINK_DOWN;
>
>       rxe_port_event(rxe, IB_EVENT_PORT_ERR);
>       dev_info(&rxe->ib_dev.dev, "set down\n");
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 9c19f2027511..4eef87c103b2 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -71,6 +71,14 @@ static int rxe_query_port(struct ib_device *dev,
>       mutex_lock(&rxe->usdev_lock);
>       rc = ib_get_eth_speed(dev, port_num, &attr->active_speed,
>                             &attr->active_width);
> +
> +     if (attr->state == IB_PORT_ACTIVE)
> +             attr->phys_state = RDMA_LINK_PHYS_STATE_LINK_UP;
Sorry. Phy state should the IB device physical state. It should depend
on the HW.
I am not sure it is a good idea to use other variable to control.

Zhu Yanjun

Hello Zhu,
That's what this is doing. If the port is admin down ('ip link set ens161 down') it will report 3 (DISABLED, despite the original #defines). If the port is not admin disabled but just without link, it will report 2 (POLLING). This is closer to the actual physical state of the hardware than what's reported today.

-Andrew

> +     else if (dev_get_flags(rxe->ndev) & IFF_UP)
> +             attr->phys_state = RDMA_LINK_PHYS_STATE_POLLING;
> +     else
> +             attr->phys_state = RDMA_LINK_PHYS_STATE_DISABLED;
> +
>       mutex_unlock(&rxe->usdev_lock);
>
>   out:
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index d9ec2de68738..8f79bd86d033 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -65,8 +65,9 @@ 
  */
 #define RXE_UVERBS_ABI_VERSION		2
 
-#define IB_PHYS_STATE_LINK_UP		(5)
-#define IB_PHYS_STATE_LINK_DOWN		(3)
+#define RDMA_LINK_PHYS_STATE_LINK_UP	(5)
+#define RDMA_LINK_PHYS_STATE_DISABLED	(3)
+#define RDMA_LINK_PHYS_STATE_POLLING	(2)
 
 #define RXE_ROCE_V2_SPORT		(0xc000)
 
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 40e82e0f6c2d..cb028a3d8275 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -607,7 +607,6 @@  void rxe_port_up(struct rxe_dev *rxe)
 
 	port = &rxe->port;
 	port->attr.state = IB_PORT_ACTIVE;
-	port->attr.phys_state = IB_PHYS_STATE_LINK_UP;
 
 	rxe_port_event(rxe, IB_EVENT_PORT_ACTIVE);
 	dev_info(&rxe->ib_dev.dev, "set active\n");
@@ -620,7 +619,6 @@  void rxe_port_down(struct rxe_dev *rxe)
 
 	port = &rxe->port;
 	port->attr.state = IB_PORT_DOWN;
-	port->attr.phys_state = IB_PHYS_STATE_LINK_DOWN;
 
 	rxe_port_event(rxe, IB_EVENT_PORT_ERR);
 	dev_info(&rxe->ib_dev.dev, "set down\n");
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 9c19f2027511..4eef87c103b2 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -71,6 +71,14 @@  static int rxe_query_port(struct ib_device *dev,
 	mutex_lock(&rxe->usdev_lock);
 	rc = ib_get_eth_speed(dev, port_num, &attr->active_speed,
 			      &attr->active_width);
+
+	if (attr->state == IB_PORT_ACTIVE)
+		attr->phys_state = RDMA_LINK_PHYS_STATE_LINK_UP;
+	else if (dev_get_flags(rxe->ndev) & IFF_UP)
+		attr->phys_state = RDMA_LINK_PHYS_STATE_POLLING;
+	else
+		attr->phys_state = RDMA_LINK_PHYS_STATE_DISABLED;
+
 	mutex_unlock(&rxe->usdev_lock);
 
 out: