[PATCH] forcedeth: reconfigure multicast packet filter only when needed

[PATCH] forcedeth: reconfigure multicast packet filter only when needed

[Jindrich Makovicka]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

Currently, the forcedeth driver reconfigures the packet filter every
time a multicast stream is (un-)subscribed. As the receiving has to be
stopped and started in this case, any multicast subscription can cause
packet loss, allowing userspace applications to disrupt incoming
traffic.

With the following patch, nv_set_multicast first checks the cached
state of the packet filter, and skips the reconfiguration if the state
does not change. With the default settings, this can reduce some
useless reconfiguration attempts. When switched to promiscuous mode,
all reconfigurations are skipped with the patch, which can be used to
mitigate packet loss problems when receiving and re-subscribing many
multicasts simultaneously on a single machine.

Signed-off-by: Jindrich Makovicka <makovick@gmail.com>

-- 
Jindrich Makovicka

[-- Attachment #2: forcedeth.c.diff --]
[-- Type: text/x-patch, Size: 2212 bytes --]

--- forcedeth.c.orig	2010-04-26 16:48:30.000000000 +0200
+++ forcedeth.c	2010-05-21 13:22:25.705907294 +0200
@@ -837,6 +837,11 @@
 	char name_rx[IFNAMSIZ + 3];       /* -rx    */
 	char name_tx[IFNAMSIZ + 3];       /* -tx    */
 	char name_other[IFNAMSIZ + 6];    /* -other */
+
+	/* current packet filter state */
+	u32 cur_pff;
+	u32 cur_addr[2];
+	u32 cur_mask[2];
 };
 
 /*
@@ -3128,17 +3133,28 @@
 	}
 	addr[0] |= NVREG_MCASTADDRA_FORCE;
 	pff |= NVREG_PFF_ALWAYS;
-	spin_lock_irq(&np->lock);
-	nv_stop_rx(dev);
-	writel(addr[0], base + NvRegMulticastAddrA);
-	writel(addr[1], base + NvRegMulticastAddrB);
-	writel(mask[0], base + NvRegMulticastMaskA);
-	writel(mask[1], base + NvRegMulticastMaskB);
-	writel(pff, base + NvRegPacketFilterFlags);
-	dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
-		dev->name);
-	nv_start_rx(dev);
-	spin_unlock_irq(&np->lock);
+	if (np->cur_pff != (pff & ~NVREG_PFF_PAUSE_RX)
+	    || memcmp(np->cur_addr, addr, sizeof(np->cur_addr)) != 0
+	    || memcmp(np->cur_mask, mask, sizeof(np->cur_mask)) != 0)
+	{
+		dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
+			dev->name);
+		spin_lock_irq(&np->lock);
+		nv_stop_rx(dev);
+		writel(addr[0], base + NvRegMulticastAddrA);
+		writel(addr[1], base + NvRegMulticastAddrB);
+		writel(mask[0], base + NvRegMulticastMaskA);
+		writel(mask[1], base + NvRegMulticastMaskB);
+		writel(pff, base + NvRegPacketFilterFlags);
+		nv_start_rx(dev);
+		spin_unlock_irq(&np->lock);
+		memcpy(np->cur_addr, addr, sizeof(np->cur_addr));
+		memcpy(np->cur_mask, mask, sizeof(np->cur_mask));
+		np->cur_pff = pff & ~NVREG_PFF_PAUSE_RX;
+	} else {
+		dprintk(KERN_INFO "%s: pff state unchanged - skipping reconfiguration.\n",
+			dev->name);
+	}
 }
 
 static void nv_update_pause(struct net_device *dev, u32 pause_flags)
@@ -5369,6 +5385,12 @@
 	writel(NVREG_MCASTMASKB_NONE, base + NvRegMulticastMaskB);
 	writel(0, base + NvRegPacketFilterFlags);
 
+	np->cur_pff = 0;
+	np->cur_addr[0] = NVREG_MCASTADDRA_FORCE;
+	np->cur_addr[1] = 0;
+	np->cur_mask[0] = NVREG_MCASTMASKA_NONE;
+	np->cur_mask[1] = NVREG_MCASTMASKB_NONE;
+
 	writel(0, base + NvRegTransmitterControl);
 	writel(0, base + NvRegReceiverControl);
 

Fwd: [PATCH] forcedeth: reconfigure multicast packet filter only when needed

[Jindřich Makovička]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

Currently, the forcedeth driver reconfigures the packet filter every
time a multicast stream is (un-)subscribed. As the receiving has to be
stopped and started in this case, any multicast subscription can cause
packet loss, allowing userspace applications to disrupt incoming
traffic.

With the following patch, nv_set_multicast first checks the cached
state of the packet filter, and skips the reconfiguration if the state
does not change. With the default settings, this can reduce some
useless reconfiguration attempts. When switched to promiscuous mode,
all reconfigurations are skipped with the patch, which can be used to
mitigate packet loss problems when receiving and re-subscribing many
multicasts simultaneously on a single machine.

Signed-off-by: Jindrich Makovicka <makovick@gmail.com>

-- 
Jindrich Makovicka

[-- Attachment #2: forcedeth.c.diff --]
[-- Type: text/x-patch, Size: 2212 bytes --]

--- forcedeth.c.orig	2010-04-26 16:48:30.000000000 +0200
+++ forcedeth.c	2010-05-21 13:22:25.705907294 +0200
@@ -837,6 +837,11 @@
 	char name_rx[IFNAMSIZ + 3];       /* -rx    */
 	char name_tx[IFNAMSIZ + 3];       /* -tx    */
 	char name_other[IFNAMSIZ + 6];    /* -other */
+
+	/* current packet filter state */
+	u32 cur_pff;
+	u32 cur_addr[2];
+	u32 cur_mask[2];
 };
 
 /*
@@ -3128,17 +3133,28 @@
 	}
 	addr[0] |= NVREG_MCASTADDRA_FORCE;
 	pff |= NVREG_PFF_ALWAYS;
-	spin_lock_irq(&np->lock);
-	nv_stop_rx(dev);
-	writel(addr[0], base + NvRegMulticastAddrA);
-	writel(addr[1], base + NvRegMulticastAddrB);
-	writel(mask[0], base + NvRegMulticastMaskA);
-	writel(mask[1], base + NvRegMulticastMaskB);
-	writel(pff, base + NvRegPacketFilterFlags);
-	dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
-		dev->name);
-	nv_start_rx(dev);
-	spin_unlock_irq(&np->lock);
+	if (np->cur_pff != (pff & ~NVREG_PFF_PAUSE_RX)
+	    || memcmp(np->cur_addr, addr, sizeof(np->cur_addr)) != 0
+	    || memcmp(np->cur_mask, mask, sizeof(np->cur_mask)) != 0)
+	{
+		dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
+			dev->name);
+		spin_lock_irq(&np->lock);
+		nv_stop_rx(dev);
+		writel(addr[0], base + NvRegMulticastAddrA);
+		writel(addr[1], base + NvRegMulticastAddrB);
+		writel(mask[0], base + NvRegMulticastMaskA);
+		writel(mask[1], base + NvRegMulticastMaskB);
+		writel(pff, base + NvRegPacketFilterFlags);
+		nv_start_rx(dev);
+		spin_unlock_irq(&np->lock);
+		memcpy(np->cur_addr, addr, sizeof(np->cur_addr));
+		memcpy(np->cur_mask, mask, sizeof(np->cur_mask));
+		np->cur_pff = pff & ~NVREG_PFF_PAUSE_RX;
+	} else {
+		dprintk(KERN_INFO "%s: pff state unchanged - skipping reconfiguration.\n",
+			dev->name);
+	}
 }
 
 static void nv_update_pause(struct net_device *dev, u32 pause_flags)
@@ -5369,6 +5385,12 @@
 	writel(NVREG_MCASTMASKB_NONE, base + NvRegMulticastMaskB);
 	writel(0, base + NvRegPacketFilterFlags);
 
+	np->cur_pff = 0;
+	np->cur_addr[0] = NVREG_MCASTADDRA_FORCE;
+	np->cur_addr[1] = 0;
+	np->cur_mask[0] = NVREG_MCASTMASKA_NONE;
+	np->cur_mask[1] = NVREG_MCASTMASKB_NONE;
+
 	writel(0, base + NvRegTransmitterControl);
 	writel(0, base + NvRegReceiverControl);
 

Re: [PATCH] forcedeth: reconfigure multicast packet filter only when needed

[Stephen Hemminger]

On Sat, 9 Oct 2010 13:26:05 +0200
Jindřich Makovička <makovick@gmail.com> wrote:

> +
> +	/* current packet filter state */
> +	u32 cur_pff;
> +	u32 cur_addr[2];
> +	u32 cur_mask[2];
>  };

No big deal, but couldn't you just put those temporary variables
on the stack. and reread the current value before stopping.

-- 

Re: [PATCH] forcedeth: reconfigure multicast packet filter only when needed

[Jindřich Makovička]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

2010/10/9 Stephen Hemminger <shemminger@vyatta.com>:
> On Sat, 9 Oct 2010 13:26:05 +0200
> Jindřich Makovička <makovick@gmail.com> wrote:
>
>> +
>> +     /* current packet filter state */
>> +     u32 cur_pff;
>> +     u32 cur_addr[2];
>> +     u32 cur_mask[2];
>>  };
>
> No big deal, but couldn't you just put those temporary variables
> on the stack. and reread the current value before stopping.

Sure, this version should do the same (still untested, I don't have a
machine with nForce here at the moment). I just wanted to avoid more
NIC register accesses, but it's probably a premature optimization.

Regards,
-- 
Jindrich Makovicka

[-- Attachment #2: forcedeth2.diff --]
[-- Type: text/x-patch, Size: 2001 bytes --]

--- forcedeth.c.orig	2010-10-07 08:56:55.564511153 +0200
+++ forcedeth.c	2010-10-09 22:34:53.151523511 +0200
@@ -3027,9 +3027,15 @@
 {
 	struct fe_priv *np = netdev_priv(dev);
 	u8 __iomem *base = get_hwbase(dev);
-	u32 addr[2];
-	u32 mask[2];
-	u32 pff = readl(base + NvRegPacketFilterFlags) & NVREG_PFF_PAUSE_RX;
+	u32 addr[2], prev_addr[2];
+	u32 mask[2], prev_mask[2];
+	u32 prev_pff = readl(base + NvRegPacketFilterFlags);
+	u32 pff = prev_pff & NVREG_PFF_PAUSE_RX;
+
+	prev_addr[0] = readl(base + NvRegMulticastAddrA);
+	prev_addr[1] = readl(base + NvRegMulticastAddrB);
+	prev_mask[0] = readl(base + NvRegMulticastMaskA);
+	prev_mask[1] = readl(base + NvRegMulticastMaskB);
 
 	memset(addr, 0, sizeof(addr));
 	memset(mask, 0, sizeof(mask));
@@ -3072,17 +3078,25 @@
 	}
 	addr[0] |= NVREG_MCASTADDRA_FORCE;
 	pff |= NVREG_PFF_ALWAYS;
-	spin_lock_irq(&np->lock);
-	nv_stop_rx(dev);
-	writel(addr[0], base + NvRegMulticastAddrA);
-	writel(addr[1], base + NvRegMulticastAddrB);
-	writel(mask[0], base + NvRegMulticastMaskA);
-	writel(mask[1], base + NvRegMulticastMaskB);
-	writel(pff, base + NvRegPacketFilterFlags);
-	dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
-		dev->name);
-	nv_start_rx(dev);
-	spin_unlock_irq(&np->lock);
+	if (prev_pff != pff
+	    || memcmp(prev_addr, addr, sizeof(prev_addr)) != 0
+	    || memcmp(prev_mask, mask, sizeof(prev_mask)) != 0)
+	{
+		dprintk(KERN_INFO "%s: reconfiguration for multicast lists.\n",
+			dev->name);
+		spin_lock_irq(&np->lock);
+		nv_stop_rx(dev);
+		writel(addr[0], base + NvRegMulticastAddrA);
+		writel(addr[1], base + NvRegMulticastAddrB);
+		writel(mask[0], base + NvRegMulticastMaskA);
+		writel(mask[1], base + NvRegMulticastMaskB);
+		writel(pff, base + NvRegPacketFilterFlags);
+		nv_start_rx(dev);
+		spin_unlock_irq(&np->lock);
+	} else {
+		dprintk(KERN_INFO "%s: pff state unchanged - skipping reconfiguration.\n",
+			dev->name);
+	}
 }
 
 static void nv_update_pause(struct net_device *dev, u32 pause_flags)