Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1697253 > unrolled thread

[PATCH] drivers/rxe: improve rxe loopback

Back to article view | Back to linux.kernel

Contents

  [PATCH] drivers/rxe: improve rxe loopback Marcel Apfelbaum <marcel@redhat.com> - 2017-07-26 17:00 +0200
    Re: [PATCH] drivers/rxe: improve rxe loopback Yuval Shaia <yuval.shaia@oracle.com> - 2017-07-26 21:40 +0200
      Re: [PATCH] drivers/rxe: improve rxe loopback Yuval Shaia <yuval.shaia@oracle.com> - 2017-07-26 22:00 +0200
    Re: [PATCH] drivers/rxe: improve rxe loopback Yuval Shaia <yuval.shaia@oracle.com> - 2017-07-26 22:00 +0200
      Re: [PATCH] drivers/rxe: improve rxe loopback Moni Shoua <monis@mellanox.com> - 2017-07-27 09:10 +0200
        Re: [PATCH] drivers/rxe: improve rxe loopback Marcel Apfelbaum <marcel@redhat.com> - 2017-07-27 12:00 +0200
          Re: [PATCH] drivers/rxe: improve rxe loopback Moni Shoua <monis@mellanox.com> - 2017-07-30 12:00 +0200
            Re: [PATCH] drivers/rxe: improve rxe loopback Marcel Apfelbaum <marcel@redhat.com> - 2017-07-31 12:00 +0200
    Re: [PATCH] drivers/rxe: improve rxe loopback Leon Romanovsky <leon@kernel.org> - 2017-07-27 09:40 +0200
      Re: [PATCH] drivers/rxe: improve rxe loopback Marcel Apfelbaum <marcel@redhat.com> - 2017-07-27 11:50 +0200
        Re: [PATCH] drivers/rxe: improve rxe loopback Leon Romanovsky <leon@kernel.org> - 2017-07-27 12:50 +0200

#1697253 — [PATCH] drivers/rxe: improve rxe loopback

Currently a packet is marked for loopback only if the source and
destination address match. This is not enough when multiple
gids are present in rxe's gid table and the traffic is
from one gid to another.

Fix it by marking the packet for loopback if the destination
address appears in rxe's gid table.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index c3a140e..b76a9a3 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
 	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
 }
 
+static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
+{
+	struct in_device *in_dev;
+	bool same_rxe = false;
+
+	rcu_read_lock();
+	in_dev = __in_dev_get_rcu(rxe->ndev);
+	if (!in_dev)
+		goto out;
+
+	for_ifa(in_dev)
+		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
+			same_rxe = true;
+			goto out;
+		}
+	endfor_ifa(in_dev);
+out:
+	rcu_read_unlock();
+	return same_rxe;
+}
+
 static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
 		    struct sk_buff *skb, struct rxe_av *av)
 {
@@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
 		return -EHOSTUNREACH;
 	}
 
-	if (!memcmp(saddr, daddr, sizeof(*daddr)))
+	if (addr4_same_rxe(rxe, daddr))
 		pkt->mask |= RXE_LOOPBACK_MASK;
 
 	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
@@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
 	return 0;
 }
 
+static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
+{
+	struct inet6_dev *in6_dev;
+	struct inet6_ifaddr *ifp;
+	bool same_rxe = false;
+
+	in6_dev = in6_dev_get(rxe->ndev);
+	if (!in6_dev)
+		return false;
+
+	read_lock_bh(&in6_dev->lock);
+	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
+		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
+			same_rxe = true;
+			goto out;
+		}
+out:
+	read_unlock_bh(&in6_dev->lock);
+	in6_dev_put(in6_dev);
+	return same_rxe;
+}
+
 static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
 		    struct sk_buff *skb, struct rxe_av *av)
 {
@@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
 		return -EHOSTUNREACH;
 	}
 
-	if (!memcmp(saddr, daddr, sizeof(*daddr)))
+	if (addr6_same_rxe(rxe, daddr))
 		pkt->mask |= RXE_LOOPBACK_MASK;
 
 	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
-- 
2.9.4

[toc] | [next] | [standalone]

#1697512

On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
> Currently a packet is marked for loopback only if the source and
> destination address match. This is not enough when multiple
> gids are present in rxe's gid table and the traffic is
> from one gid to another.
> 
> Fix it by marking the packet for loopback if the destination
> address appears in rxe's gid table.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c3a140e..b76a9a3 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
>  	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
>  }
>  
> +static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
> +{
> +	struct in_device *in_dev;
> +	bool same_rxe = false;
> +
> +	rcu_read_lock();
> +	in_dev = __in_dev_get_rcu(rxe->ndev);
> +	if (!in_dev)
> +		goto out;
> +
> +	for_ifa(in_dev)
> +		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +	endfor_ifa(in_dev);

The above endfor_ifa should move to below.

> +out:
> +	rcu_read_unlock();
> +	return same_rxe;
> +}
> +
>  static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>  
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr4_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>  
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> @@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  	return 0;
>  }
>  
> +static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
> +{
> +	struct inet6_dev *in6_dev;
> +	struct inet6_ifaddr *ifp;
> +	bool same_rxe = false;
> +
> +	in6_dev = in6_dev_get(rxe->ndev);
> +	if (!in6_dev)
> +		return false;
> +
> +	read_lock_bh(&in6_dev->lock);
> +	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
> +		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +out:
> +	read_unlock_bh(&in6_dev->lock);
> +	in6_dev_put(in6_dev);
> +	return same_rxe;
> +}
> +
>  static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>  
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr6_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>  
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> -- 
> 2.9.4
> 

[toc] | [prev] | [next] | [standalone]

#1697527

> > +	endfor_ifa(in_dev);
> 
> The above endfor_ifa should move to below.

Please ignore, my mistake.

> 
> > +out:

[toc] | [prev] | [next] | [standalone]

#1697523

On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
> Currently a packet is marked for loopback only if the source and
> destination address match. This is not enough when multiple
> gids are present in rxe's gid table and the traffic is
> from one gid to another.
> 
> Fix it by marking the packet for loopback if the destination
> address appears in rxe's gid table.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

Tested-by: Yuval Shaia <yuval.shaia@oracle.com>

> ---
>  drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c3a140e..b76a9a3 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
>  	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
>  }
>  
> +static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
> +{
> +	struct in_device *in_dev;
> +	bool same_rxe = false;
> +
> +	rcu_read_lock();
> +	in_dev = __in_dev_get_rcu(rxe->ndev);
> +	if (!in_dev)
> +		goto out;
> +
> +	for_ifa(in_dev)
> +		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +	endfor_ifa(in_dev);
> +out:
> +	rcu_read_unlock();
> +	return same_rxe;
> +}
> +
>  static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>  
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr4_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>  
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> @@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  	return 0;
>  }
>  
> +static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
> +{
> +	struct inet6_dev *in6_dev;
> +	struct inet6_ifaddr *ifp;
> +	bool same_rxe = false;
> +
> +	in6_dev = in6_dev_get(rxe->ndev);
> +	if (!in6_dev)
> +		return false;
> +
> +	read_lock_bh(&in6_dev->lock);
> +	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
> +		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +out:
> +	read_unlock_bh(&in6_dev->lock);
> +	in6_dev_put(in6_dev);
> +	return same_rxe;
> +}
> +
>  static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>  
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr6_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>  
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> -- 
> 2.9.4
> 

[toc] | [prev] | [next] | [standalone]

#1697761

On Wed, Jul 26, 2017 at 10:57 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote:
> On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
>> Currently a packet is marked for loopback only if the source and
>> destination address match. This is not enough when multiple
>> gids are present in rxe's gid table and the traffic is
>> from one gid to another.
>>
>> Fix it by marking the packet for loopback if the destination
>> address appears in rxe's gid table.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
Have you considered using ip_route_output_key() for IPv4 or
ip6_route_output() for IPv6 to decide if  this is a loopback?
For reference you can check the flow starting at  rdma_resolve_ip()

[toc] | [prev] | [next] | [standalone]

#1697857

On 27/07/2017 10:04, Moni Shoua wrote:
> On Wed, Jul 26, 2017 at 10:57 PM, Yuval Shaia <yuval.shaia@oracle.com> wrote:
>> On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
>>> Currently a packet is marked for loopback only if the source and
>>> destination address match. This is not enough when multiple
>>> gids are present in rxe's gid table and the traffic is
>>> from one gid to another.
>>>
>>> Fix it by marking the packet for loopback if the destination
>>> address appears in rxe's gid table.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>
> Have you considered using ip_route_output_key() for IPv4 or
> ip6_route_output() for IPv6 to decide if  this is a loopback?
> For reference you can check the flow starting at  rdma_resolve_ip()
> 

Hi Moni,

Yes, I had looked into it, but I haven't seen how I can find
out if the destination IP belongs to the same RXE.
The loopback flag will give us the "same host"
confirmation, but not the same rxe instance, right?

Any ideas would be welcomed.

Thanks,
Marcel

[toc] | [prev] | [next] | [standalone]

#1699492

>> Have you considered using ip_route_output_key() for IPv4 or
>> ip6_route_output() for IPv6 to decide if  this is a loopback?
>> For reference you can check the flow starting at  rdma_resolve_ip()
>>
>
> Hi Moni,
>
> Yes, I had looked into it, but I haven't seen how I can find
> out if the destination IP belongs to the same RXE.
> The loopback flag will give us the "same host"
> confirmation, but not the same rxe instance, right?
>
> Any ideas would be welcomed.
>
> Thanks,
> Marcel
>
Hi Marcel

You are right about that. IFF_LOOPBACK tells you that the source and
destination addresses are on the same host but not necessarily on the
same RXE device.

As Leon mentioned, calling addrX_same_rxe() for each packet seems to
heavy , especially when the use case that justifies it (instead of
calling memcmp() on src and dst) is rare. Do you agree?
If so I think that marking a connection as loopback once is the right approach
For RC/UC - when modified to RTR
For UD - this is harder. IsLoopback() is function of the WQE (or at
least the QP and AH together( but not the QP. I think you can add an
improvement that will work for the majority of cases. This is a sketch
of what I have in mind. Let me know what you think please

1. Add bool last_used_qp to AH structure
2. Add bool is_loopback_with_last_qp to AH structure
3. Set values to AH.last_used_qp and AH.is_loopback_with_last_qp in
post_send() modify_ah(),...
4. Mark WQE as loopback depending on the above

[toc] | [prev] | [next] | [standalone]

#1699868

On 30/07/2017 12:57, Moni Shoua wrote:
>>> Have you considered using ip_route_output_key() for IPv4 or
>>> ip6_route_output() for IPv6 to decide if  this is a loopback?
>>> For reference you can check the flow starting at  rdma_resolve_ip()
>>>
>>
>> Hi Moni,
>>
>> Yes, I had looked into it, but I haven't seen how I can find
>> out if the destination IP belongs to the same RXE.
>> The loopback flag will give us the "same host"
>> confirmation, but not the same rxe instance, right?
>>
>> Any ideas would be welcomed.
>>
>> Thanks,
>> Marcel
>>
> Hi Marcel
> 

Hi Moni,

> You are right about that. IFF_LOOPBACK tells you that the source and
> destination addresses are on the same host but not necessarily on the
> same RXE device.
> 
> As Leon mentioned, calling addrX_same_rxe() for each packet seems to
> heavy , especially when the use case that justifies it (instead of
> calling memcmp() on src and dst) is rare. Do you agree?

I do agree is rare, but is depending on use-case. And since it
is a bug we should fix it, but not on the expense of performance
of course.

> If so I think that marking a connection as loopback once is the right approach
> For RC/UC - when modified to RTR

Sounds good to me.

> For UD - this is harder. IsLoopback() is function of the WQE (or at
> least the QP and AH together( but not the QP. I think you can add an
> improvement that will work for the majority of cases. This is a sketch
> of what I have in mind. Let me know what you think please
> 
> 1. Add bool last_used_qp to AH structure
> 2. Add bool is_loopback_with_last_qp to AH structure
> 3. Set values to AH.last_used_qp and AH.is_loopback_with_last_qp in
> post_send() modify_ah(),...
> 4. Mark WQE as loopback depending on the above
> 

Your pointer is very much appreciated, I will look into it.

Thanks,
Marcel

[toc] | [prev] | [next] | [standalone]

#1697773

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
> Currently a packet is marked for loopback only if the source and
> destination address match. This is not enough when multiple
> gids are present in rxe's gid table and the traffic is
> from one gid to another.
>
> Fix it by marking the packet for loopback if the destination
> address appears in rxe's gid table.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index c3a140e..b76a9a3 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
>  	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
>  }
>
> +static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
> +{

In addition to Moni's comment, no "inline" functions in *.c files, please.

> +	struct in_device *in_dev;
> +	bool same_rxe = false;
> +
> +	rcu_read_lock();
> +	in_dev = __in_dev_get_rcu(rxe->ndev);
> +	if (!in_dev)
> +		goto out;
> +
> +	for_ifa(in_dev)
> +		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +	endfor_ifa(in_dev);

I'm afraid that it will decrease performance drastically. One of the
possible solutions to overcome it, is to check the address of first packet
only, but it will work for RC only.

> +out:
> +	rcu_read_unlock();
> +	return same_rxe;
> +}
> +
>  static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr4_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> @@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  	return 0;
>  }
>
> +static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
> +{

Ditto

> +	struct inet6_dev *in6_dev;
> +	struct inet6_ifaddr *ifp;
> +	bool same_rxe = false;
> +
> +	in6_dev = in6_dev_get(rxe->ndev);
> +	if (!in6_dev)
> +		return false;
> +
> +	read_lock_bh(&in6_dev->lock);
> +	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
> +		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
> +			same_rxe = true;
> +			goto out;
> +		}
> +out:
> +	read_unlock_bh(&in6_dev->lock);
> +	in6_dev_put(in6_dev);
> +	return same_rxe;
> +}
> +
>  static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		    struct sk_buff *skb, struct rxe_av *av)
>  {
> @@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>  		return -EHOSTUNREACH;
>  	}
>
> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> +	if (addr6_same_rxe(rxe, daddr))
>  		pkt->mask |= RXE_LOOPBACK_MASK;
>
>  	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1697849

On 27/07/2017 10:36, Leon Romanovsky wrote:
> On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
>> Currently a packet is marked for loopback only if the source and
>> destination address match. This is not enough when multiple
>> gids are present in rxe's gid table and the traffic is
>> from one gid to another.
>>
>> Fix it by marking the packet for loopback if the destination
>> address appears in rxe's gid table.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
>>   1 file changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
>> index c3a140e..b76a9a3 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_net.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
>> @@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
>>   	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
>>   }
>>
>> +static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
>> +{

Hi Leon,
Thanks for the review.

> 
> In addition to Moni's comment, no "inline" functions in *.c files, please.
> 

Sure, I simply followed the function on the same file:
   static inline int addr_same(struct rxe_dev *rxe, struct rxe_av *av)
I even borrowed the name...

>> +	struct in_device *in_dev;
>> +	bool same_rxe = false;
>> +
>> +	rcu_read_lock();
>> +	in_dev = __in_dev_get_rcu(rxe->ndev);
>> +	if (!in_dev)
>> +		goto out;
>> +
>> +	for_ifa(in_dev)
>> +		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
>> +			same_rxe = true;
>> +			goto out;
>> +		}
>> +	endfor_ifa(in_dev);
> 
> I'm afraid that it will decrease performance drastically. One of the
> possible solutions to overcome it, is to check the address of first packet
> only, but it will work for RC only.
> 

How do you know is "the first" packet?
And yes, for UD the performance would decrease, but only
if the netdev has multiple IPs, right?

I'll ask on Moni's response mail for alternatives.

Thanks,
Marcel

>> +out:
>> +	rcu_read_unlock();
>> +	return same_rxe;
>> +}
>> +
>>   static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>>   		    struct sk_buff *skb, struct rxe_av *av)
>>   {
>> @@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>>   		return -EHOSTUNREACH;
>>   	}
>>
>> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
>> +	if (addr4_same_rxe(rxe, daddr))
>>   		pkt->mask |= RXE_LOOPBACK_MASK;
>>
>>   	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
>> @@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>>   	return 0;
>>   }
>>
>> +static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
>> +{
> 
> Ditto
> 
>> +	struct inet6_dev *in6_dev;
>> +	struct inet6_ifaddr *ifp;
>> +	bool same_rxe = false;
>> +
>> +	in6_dev = in6_dev_get(rxe->ndev);
>> +	if (!in6_dev)
>> +		return false;
>> +
>> +	read_lock_bh(&in6_dev->lock);
>> +	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
>> +		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
>> +			same_rxe = true;
>> +			goto out;
>> +		}
>> +out:
>> +	read_unlock_bh(&in6_dev->lock);
>> +	in6_dev_put(in6_dev);
>> +	return same_rxe;
>> +}
>> +
>>   static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>>   		    struct sk_buff *skb, struct rxe_av *av)
>>   {
>> @@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
>>   		return -EHOSTUNREACH;
>>   	}
>>
>> -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
>> +	if (addr6_same_rxe(rxe, daddr))
>>   		pkt->mask |= RXE_LOOPBACK_MASK;
>>
>>   	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
>> --
>> 2.9.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1697885

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 27, 2017 at 12:49:17PM +0300, Marcel Apfelbaum wrote:
> On 27/07/2017 10:36, Leon Romanovsky wrote:
> > On Wed, Jul 26, 2017 at 05:52:48PM +0300, Marcel Apfelbaum wrote:
> > > Currently a packet is marked for loopback only if the source and
> > > destination address match. This is not enough when multiple
> > > gids are present in rxe's gid table and the traffic is
> > > from one gid to another.
> > >
> > > Fix it by marking the packet for loopback if the destination
> > > address appears in rxe's gid table.
> > >
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > >   drivers/infiniband/sw/rxe/rxe_net.c | 47 +++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 45 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> > > index c3a140e..b76a9a3 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_net.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> > > @@ -351,6 +351,27 @@ static void prepare_ipv6_hdr(struct dst_entry *dst, struct sk_buff *skb,
> > >   	ip6h->payload_len = htons(skb->len - sizeof(*ip6h));
> > >   }
> > >
> > > +static inline bool addr4_same_rxe(struct rxe_dev *rxe, struct in_addr *daddr)
> > > +{
>
> Hi Leon,
> Thanks for the review.
>
> >
> > In addition to Moni's comment, no "inline" functions in *.c files, please.
> >
>
> Sure, I simply followed the function on the same file:
>   static inline int addr_same(struct rxe_dev *rxe, struct rxe_av *av)
> I even borrowed the name...
>
> > > +	struct in_device *in_dev;
> > > +	bool same_rxe = false;
> > > +
> > > +	rcu_read_lock();
> > > +	in_dev = __in_dev_get_rcu(rxe->ndev);
> > > +	if (!in_dev)
> > > +		goto out;
> > > +
> > > +	for_ifa(in_dev)
> > > +		if (!memcmp(&ifa->ifa_address, daddr, sizeof(*daddr))) {
> > > +			same_rxe = true;
> > > +			goto out;
> > > +		}
> > > +	endfor_ifa(in_dev);
> >
> > I'm afraid that it will decrease performance drastically. One of the
> > possible solutions to overcome it, is to check the address of first packet
> > only, but it will work for RC only.
> >
>
> How do you know is "the first" packet?
> And yes, for UD the performance would decrease, but only
> if the netdev has multiple IPs, right?

Yes, and first lookup for QP RC will be "first packet". QP RC are created with "static" address.

>
> I'll ask on Moni's response mail for alternatives.
>
> Thanks,
> Marcel
>
> > > +out:
> > > +	rcu_read_unlock();
> > > +	return same_rxe;
> > > +}
> > > +
> > >   static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
> > >   		    struct sk_buff *skb, struct rxe_av *av)
> > >   {
> > > @@ -367,7 +388,7 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
> > >   		return -EHOSTUNREACH;
> > >   	}
> > >
> > > -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> > > +	if (addr4_same_rxe(rxe, daddr))
> > >   		pkt->mask |= RXE_LOOPBACK_MASK;
> > >
> > >   	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> > > @@ -384,6 +405,28 @@ static int prepare4(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
> > >   	return 0;
> > >   }
> > >
> > > +static inline bool addr6_same_rxe(struct rxe_dev *rxe, struct in6_addr *daddr)
> > > +{
> >
> > Ditto
> >
> > > +	struct inet6_dev *in6_dev;
> > > +	struct inet6_ifaddr *ifp;
> > > +	bool same_rxe = false;
> > > +
> > > +	in6_dev = in6_dev_get(rxe->ndev);
> > > +	if (!in6_dev)
> > > +		return false;
> > > +
> > > +	read_lock_bh(&in6_dev->lock);
> > > +	list_for_each_entry(ifp, &in6_dev->addr_list, if_list)
> > > +		if (!memcmp(&ifp->addr, daddr, sizeof(*daddr))) {
> > > +			same_rxe = true;
> > > +			goto out;
> > > +		}
> > > +out:
> > > +	read_unlock_bh(&in6_dev->lock);
> > > +	in6_dev_put(in6_dev);
> > > +	return same_rxe;
> > > +}
> > > +
> > >   static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
> > >   		    struct sk_buff *skb, struct rxe_av *av)
> > >   {
> > > @@ -398,7 +441,7 @@ static int prepare6(struct rxe_dev *rxe, struct rxe_pkt_info *pkt,
> > >   		return -EHOSTUNREACH;
> > >   	}
> > >
> > > -	if (!memcmp(saddr, daddr, sizeof(*daddr)))
> > > +	if (addr6_same_rxe(rxe, daddr))
> > >   		pkt->mask |= RXE_LOOPBACK_MASK;
> > >
> > >   	prepare_udp_hdr(skb, htons(RXE_ROCE_V2_SPORT),
> > > --
> > > 2.9.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web