[IPoIB] Identify multicast packets and fix IGMP breakage V2

[IPoIB] Identify multicast packets and fix IGMP breakage V2

[Christoph Lameter]

Is this better?


Subject: [IPoIB] Identify Multicast packets and fix IGMP breakage V2

IGMP processing is broken because the IPOIB does not set the
skb->pkt_type the right way for Multicast traffic. All incoming
packets are set to PACKET_HOST which means that the igmp_recv()
function will ignore the IGMP broadcasts/multicasts.

This in turn means that the IGMP timers are firing and are sending
information about multicast subscriptions unnecessarily. In a large
private network this can cause traffic spikes.

Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>

---
 drivers/infiniband/ulp/ipoib/ipoib_ib.c |   10 +++++++---
 include/linux/in6.h                     |    3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 15:11:34.000000000 -0500
+++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 16:29:19.000000000 -0500
@@ -271,6 +271,13 @@ static void ipoib_ib_handle_rx_wc(struct
 	ipoib_ud_dma_unmap_rx(priv, mapping);
 	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);

+	if ((wc->wc_flags & IB_WC_GRH) &&
+		IN6_IS_ADDR_MULTICAST(&((struct ipv6hdr *)skb->data)->daddr))
+
+		skb->pkt_type = PACKET_MULTICAST;
+	else
+		skb->pkt_type = PACKET_HOST;
+
 	skb_pull(skb, IB_GRH_BYTES);

 	skb->protocol = ((struct ipoib_header *) skb->data)->proto;
@@ -281,9 +288,6 @@ static void ipoib_ib_handle_rx_wc(struct
 	dev->stats.rx_bytes += skb->len;

 	skb->dev = dev;
-	/* XXX get correct PACKET_ type here */
-	skb->pkt_type = PACKET_HOST;
-
 	if (test_bit(IPOIB_FLAG_CSUM, &priv->flags) && likely(wc->csum_ok))
 		skb->ip_summed = CHECKSUM_UNNECESSARY;

Index: linux-2.6/include/linux/in6.h
===================================================================
--- linux-2.6.orig/include/linux/in6.h	2010-08-26 15:11:34.000000000 -0500
+++ linux-2.6/include/linux/in6.h	2010-08-26 16:27:08.000000000 -0500
@@ -53,6 +53,9 @@ extern const struct in6_addr in6addr_lin
 extern const struct in6_addr in6addr_linklocal_allrouters;
 #define IN6ADDR_LINKLOCAL_ALLROUTERS_INIT \
 		{ { { 0xff,2,0,0,0,0,0,0,0,0,0,0,0,0,0,2 } } }
+
+#define IN6_IS_ADDR_MULTICAST(a) ((a)->in6_u.u6_addr8[0] == 0xff)
+
 #endif

 struct sockaddr_in6 {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V2

[David Miller]

From: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
Date: Thu, 26 Aug 2010 16:31:14 -0500 (CDT)

> @@ -271,6 +271,13 @@ static void ipoib_ib_handle_rx_wc(struct
>  	ipoib_ud_dma_unmap_rx(priv, mapping);
>  	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);
> 
> +	if ((wc->wc_flags & IB_WC_GRH) &&
> +		IN6_IS_ADDR_MULTICAST(&((struct ipv6hdr *)skb->data)->daddr))
> +
> +		skb->pkt_type = PACKET_MULTICAST;
> +	else
> +		skb->pkt_type = PACKET_HOST;

I really don't think you can assume there is an ipv6 header here
at all.

You'll need to parse the encapsulated protocol and process it as
ipv4 or ipv6 as needed.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V2

[Jason Gunthorpe]

On Thu, Aug 26, 2010 at 03:15:53PM -0700, David Miller wrote:
> From: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
> Date: Thu, 26 Aug 2010 16:31:14 -0500 (CDT)
> 
> > @@ -271,6 +271,13 @@ static void ipoib_ib_handle_rx_wc(struct
> >  	ipoib_ud_dma_unmap_rx(priv, mapping);
> >  	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);
> > 
> > +	if ((wc->wc_flags & IB_WC_GRH) &&
> > +		IN6_IS_ADDR_MULTICAST(&((struct ipv6hdr *)skb->data)->daddr))
> > +
> > +		skb->pkt_type = PACKET_MULTICAST;
> > +	else
> > +		skb->pkt_type = PACKET_HOST;
> 
> I really don't think you can assume there is an ipv6 header here
> at all.

The 40 bytes at this location are defined by the HW specification to
be an IB GRH which has an identical layout to an IPv6 header. Roland
is right, it would be clearer to use ib_grh ->dgid

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Christoph Lameter]

On Thu, 26 Aug 2010, Jason Gunthorpe wrote:

> The 40 bytes at this location are defined by the HW specification to
> be an IB GRH which has an identical layout to an IPv6 header. Roland
> is right, it would be clearer to use ib_grh ->dgid

Ok but then we have no nice function that checks for multicast anymore.



Subject: [IPoIB] Identify multicast packets and fix IGMP breakage V3

IGMP processing is broken because the IPOIB does not set the
skb->pkt_type the right way for Multicast traffic. All incoming
packets are set to PACKET_HOST which means that the igmp_recv()
function will ignore the IGMP broadcasts/multicasts.

This in turn means that the IGMP timers are firing and are sending
information about multicast subscriptions unnecessarily. In a large
private network this can cause traffic spikes.

Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>

---

Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 18:24:07.842079559 -0500
+++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 18:25:33.859815544 -0500
@@ -271,6 +271,14 @@
 	ipoib_ud_dma_unmap_rx(priv, mapping);
 	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);

+	/* First byte of dgid signals multicast when 0xff */
+	if ((wc->wc_flags & IB_WC_GRH) &&
+		((struct ib_grh *)skb->data)->dgid.raw[0] == 0xff)
+
+		skb->pkt_type = PACKET_MULTICAST;
+	else
+		skb->pkt_type = PACKET_HOST;
+
 	skb_pull(skb, IB_GRH_BYTES);

 	skb->protocol = ((struct ipoib_header *) skb->data)->proto;
@@ -281,9 +289,6 @@
 	dev->stats.rx_bytes += skb->len;

 	skb->dev = dev;
-	/* XXX get correct PACKET_ type here */
-	skb->pkt_type = PACKET_HOST;
-
 	if (test_bit(IPOIB_FLAG_CSUM, &priv->flags) && likely(wc->csum_ok))
 		skb->ip_summed = CHECKSUM_UNNECESSARY;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Jason Gunthorpe]

On Thu, Aug 26, 2010 at 06:26:58PM -0500, Christoph Lameter wrote:
> On Thu, 26 Aug 2010, Jason Gunthorpe wrote:
>
> > The 40 bytes at this location are defined by the HW specification to
> > be an IB GRH which has an identical layout to an IPv6 header. Roland
> > is right, it would be clearer to use ib_grh ->dgid
>
> Ok but then we have no nice function that checks for multicast anymore.

Were you going to try it this way?

     /* First byte of dgid signals multicast/broadcast when 0xff */
     if ((wc->wc_flags & IB_WC_GRH) &&
         ((struct ib_grh *)skb->data)->dgid.raw[0] == 0xff) {
	     if (memcmp(((struct ib_grh *)skb->data)->dgid.raw,
                        dev->broadcast + 4, sizeof(union ib_gid)) == 0)
                     skb->pkt_type = PACKET_BROADCAST;
             else
                     skb->pkt_type = PACKET_MULTICAST;
     }
     else
             skb->pkt_type = PACKET_HOST;

I think doing the memcmp only in the multicast path should be
reasonable overhead wise.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Christoph Lameter]

On Thu, 26 Aug 2010, Jason Gunthorpe wrote:

> I think doing the memcmp only in the multicast path should be
> reasonable overhead wise.

Thats is not always possible. Here the multicast path is the
default path that is taken 99% of the time.

RE: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Yossi Etigin]

> 
> Were you going to try it this way?
> 
>      /* First byte of dgid signals multicast/broadcast when 0xff */
>      if ((wc->wc_flags & IB_WC_GRH) &&
>          ((struct ib_grh *)skb->data)->dgid.raw[0] == 0xff) {
> 	     if (memcmp(((struct ib_grh *)skb->data)->dgid.raw,
>                         dev->broadcast + 4, sizeof(union ib_gid)) ==
0)
>                      skb->pkt_type = PACKET_BROADCAST;
>              else
>                      skb->pkt_type = PACKET_MULTICAST;
>      }
>      else
>              skb->pkt_type = PACKET_HOST;
> 
> I think doing the memcmp only in the multicast path should be
> reasonable overhead wise.
> 

Shouldn't struct ib_grh be packed to make this really work?
The code looks a little messy to me anyway... 
How about using a local var which is a ptr to packed struct ib_grh? The
compiler will probably eliminate it anyway.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[David Miller]

From: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
Date: Thu, 26 Aug 2010 18:57:09 -0500 (CDT)

> On Thu, 26 Aug 2010, Jason Gunthorpe wrote:
> 
>> I think doing the memcmp only in the multicast path should be
>> reasonable overhead wise.
> 
> Thats is not always possible. Here the multicast path is the
> default path that is taken 99% of the time.

The highest cost is bringing in that packet header's cache line, which
you've already done by reading the byte and checking for 0xff.

I doubt the memcmp() can possibly matter.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Jason Gunthorpe]

On Fri, Aug 27, 2010 at 03:02:54AM +0300, Yossi Etigin wrote:

> Shouldn't struct ib_grh be packed to make this really work?

No idea what the kernel convention for this is. It looks OK to me, in
that no arch I am familiar with will insert padding.

> The code looks a little messy to me anyway... 
> How about using a local var which is a ptr to packed struct ib_grh? The
> compiler will probably eliminate it anyway.

This bike shed is looking pretty well painted already :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Christoph Lameter]

On Thu, 26 Aug 2010, David Miller wrote:

> The highest cost is bringing in that packet header's cache line, which
> you've already done by reading the byte and checking for 0xff.

And then you need to bring in the cacheline of the broadcast address....
These are pretty long byte strings in the IB case.

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Christoph Lameter]

On Thu, 26 Aug 2010, Jason Gunthorpe wrote:

> I think doing the memcmp only in the multicast path should be
> reasonable overhead wise.

Ok the dgid is only 8 bytes not the whole 40 bytes.... Here is the patch
somewhat cleaned up with PACKET_BROADCAST.


Subject: [IPoIB] Identify multicast packets and fix IGMP breakage V3

IGMP processing is broken because the IPOIB does not set the
skb->pkt_type the right way for Multicast traffic. All incoming
packets are set to PACKET_HOST which means that the igmp_recv()
function will ignore the IGMP broadcasts/multicasts.

This in turn means that the IGMP timers are firing and are sending
information about multicast subscriptions unnecessarily. In a large
private network this can cause traffic spikes.

Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>

---

Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 18:24:07.842079559 -0500
+++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-27 08:26:37.929641162 -0500
@@ -223,6 +223,7 @@
 	unsigned int wr_id = wc->wr_id & ~IPOIB_OP_RECV;
 	struct sk_buff *skb;
 	u64 mapping[IPOIB_UD_RX_SG];
+	union ib_gid *dgid;

 	ipoib_dbg_data(priv, "recv completion: id %d, status: %d\n",
 		       wr_id, wc->status);
@@ -271,6 +272,21 @@
 	ipoib_ud_dma_unmap_rx(priv, mapping);
 	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);

+	/* First byte of dgid signals multicast when 0xff */
+	dgid = &((struct ib_grh *)skb->data)->dgid;
+
+	if (!(wc->wc_flags & IB_WC_GRH) || dgid->raw[0] != 0xff)
+
+		skb->pkt_type = PACKET_HOST;
+
+	else if (memcmp(dgid, dev->broadcast + 4, sizeof(union ib_gid)) == 0)
+
+		skb->pkt_type = PACKET_BROADCAST;
+
+	else
+
+		skb->pkt_type = PACKET_MULTICAST;
+
 	skb_pull(skb, IB_GRH_BYTES);

 	skb->protocol = ((struct ipoib_header *) skb->data)->proto;
@@ -281,9 +297,6 @@
 	dev->stats.rx_bytes += skb->len;

 	skb->dev = dev;
-	/* XXX get correct PACKET_ type here */
-	skb->pkt_type = PACKET_HOST;
-
 	if (test_bit(IPOIB_FLAG_CSUM, &priv->flags) && likely(wc->csum_ok))
 		skb->ip_summed = CHECKSUM_UNNECESSARY;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Or Gerlitz]

Christoph Lameter wrote:
> Here is the patch somewhat cleaned up with PACKET_BROADCAST.
> Subject: [IPoIB] Identify multicast packets and fix IGMP breakage V3
>   

I don't see this patch in Roland's for-next branch nor Dave's 
net-next-2.6 tree, is anything else needed to merge that?

Or.

> IGMP processing is broken because the IPOIB does not set the
> skb->pkt_type the right way for Multicast traffic. All incoming
> packets are set to PACKET_HOST which means that the igmp_recv()
> function will ignore the IGMP broadcasts/multicasts.
>
> This in turn means that the IGMP timers are firing and are sending
> information about multicast subscriptions unnecessarily. In a large
> private network this can cause traffic spikes.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
>
> ---
>
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-26 18:24:07.842079559 -0500
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_ib.c	2010-08-27 08:26:37.929641162 -0500
> @@ -223,6 +223,7 @@
>  	unsigned int wr_id = wc->wr_id & ~IPOIB_OP_RECV;
>  	struct sk_buff *skb;
>  	u64 mapping[IPOIB_UD_RX_SG];
> +	union ib_gid *dgid;
>
>  	ipoib_dbg_data(priv, "recv completion: id %d, status: %d\n",
>  		       wr_id, wc->status);
> @@ -271,6 +272,21 @@
>  	ipoib_ud_dma_unmap_rx(priv, mapping);
>  	ipoib_ud_skb_put_frags(priv, skb, wc->byte_len);
>
> +	/* First byte of dgid signals multicast when 0xff */
> +	dgid = &((struct ib_grh *)skb->data)->dgid;
> +
> +	if (!(wc->wc_flags & IB_WC_GRH) || dgid->raw[0] != 0xff)
> +
> +		skb->pkt_type = PACKET_HOST;
> +
> +	else if (memcmp(dgid, dev->broadcast + 4, sizeof(union ib_gid)) == 0)
> +
> +		skb->pkt_type = PACKET_BROADCAST;
> +
> +	else
> +
> +		skb->pkt_type = PACKET_MULTICAST;
> +
>  	skb_pull(skb, IB_GRH_BYTES);
>
>  	skb->protocol = ((struct ipoib_header *) skb->data)->proto;
> @@ -281,9 +297,6 @@
>  	dev->stats.rx_bytes += skb->len;
>
>  	skb->dev = dev;
> -	/* XXX get correct PACKET_ type here */
> -	skb->pkt_type = PACKET_HOST;
> -
>  	if (test_bit(IPOIB_FLAG_CSUM, &priv->flags) && likely(wc->csum_ok))
>  		skb->ip_summed = CHECKSUM_UNNECESSARY;
>
>   

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Christoph Lameter]

On Tue, 14 Sep 2010, Or Gerlitz wrote:

> I don't see this patch in Roland's for-next branch nor Dave's net-next-2.6
> tree, is anything else needed to merge that?

No there is nothing else needed.

Roland said he is going to merge it for 2.6.37.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [IPoIB] Identify multicast packets and fix IGMP breakage V3

[Roland Dreier]

thanks, applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html