Hi
  Series v2 of 5 patches to support vlan rx accleration.
  This takes care of Eric Dumazet comment of using dev_kfree_skb(skb).
 
  I haven't combined accleration and GRO patch. Though its look strange
  with first patch. But I want to have separate patches for two different 
  entity.

-Amit
--

[ message continues ]

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index c8caec9..714ddf4 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -51,8 +51,8 @@
 
 #define _QLCNIC_LINUX_MAJOR 5
 #define _QLCNIC_LINUX_MINOR 0
-#define _QLCNIC_LINUX_SUBVERSION 9
-#define QLCNIC_LINUX_VERSIONID  "5.0.9"
+#define _QLCNIC_LINUX_SUBVERSION 10
+#define QLCNIC_LINUX_VERSIONID  "5.0.10"
 #define QLCNIC_DRV_IDC_VER  0x01
 #define QLCNIC_DRIVER_VERSION  ((_QLCNIC_LINUX_MAJOR << 16) |\
 		 (_QLCNIC_LINUX_MINOR << 8) | (_QLCNIC_LINUX_SUBVERSION))
-- 
1.6.0.2

--

[ message continues ]

Don't compare flash and file fw version. Allow to load
old fw from file than flashed fw.
If file fw is present, don't skip fw re-intialization.

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic_init.c |   30 ++----------------------------
 1 files changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 9f994b9..e26fa95 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1015,8 +1015,6 @@ qlcnic_check_fw_hearbeat(struct qlcnic_adapter *adapter)
 int
 qlcnic_need_fw_reset(struct qlcnic_adapter *adapter)
 {
-	u32 val, version, major, minor, build;
-
 	if (qlcnic_check_fw_hearbeat(adapter)) {
 		qlcnic_rom_lock_recovery(adapter);
 		return 1;
@@ -1025,20 +1023,8 @@ qlcnic_need_fw_reset(struct qlcnic_adapter *adapter)
 	if (adapter->need_fw_reset)
 		return 1;
 
-	/* check if we have got newer or different file firmware */
-	if (adapter->fw) {
-
-		val = qlcnic_get_fw_version(adapter);
-
-		version = QLCNIC_DECODE_VERSION(val);
-
-		major = QLCRD32(adapter, QLCNIC_FW_VERSION_MAJOR);
-		minor = QLCRD32(adapter, QLCNIC_FW_VERSION_MINOR);
-		build = QLCRD32(adapter, QLCNIC_FW_VERSION_SUB);
-
-		if (version > QLCNIC_VERSION_CODE(major, minor, build))
-			return 1;
-	}
+	if (adapter->fw)
+		return 1;
 
 	return 0;
 }
@@ -1174,18 +1160,6 @@ qlcnic_validate_firmware(struct qlcnic_adapter *adapter)
 		return -EINVAL;
 	}
 
-	/* check if flashed firmware is newer */
-	if (qlcnic_rom_fast_read(adapter,
-			QLCNIC_FW_VERSION_OFFSET, (int *)&val))
-		return -EIO;
-
-	val = QLCNIC_DECODE_VERSION(val);
-	if (val > ver) {
-		dev_info(&pdev->dev, "%s: firmware is older than flash\n",
-				fw_name[fw_type]);
-		return -EINVAL;
-	}
-
 	QLCWR32(adapter, QLCNIC_CAM_RAM(0x1fc), QLCNIC_BDINFO_MAGIC);
 	return 0;
 }
-- 
1.6.0.2

--

[ message continues ]

Implemented vlan rx accleration in driver.
This helps in increasing significant performance and
reduces cpu utilization with GRO and LRO.

Eric Dumazet:
	"Its a bit strange you use dev_kfree_skb_any(skb) here."
	"We run in NAPI mode, so you can use dev_kfree_skb()."
Amit:
	Done. Using dev_kfree_skb();

Signed-off-by: Amit Kumar Salecha <amit.salecha@qlogic.com>
---
 drivers/net/qlcnic/qlcnic.h      |    1 +
 drivers/net/qlcnic/qlcnic_init.c |   59 ++++++++++++++++++++++----------------
 drivers/net/qlcnic/qlcnic_main.c |   10 ++++++-
 3 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic.h b/drivers/net/qlcnic/qlcnic.h
index cc8385a..c8caec9 100644
--- a/drivers/net/qlcnic/qlcnic.h
+++ b/drivers/net/qlcnic/qlcnic.h
@@ -1013,6 +1013,7 @@ struct qlcnic_adapter {
 
 	u64 dev_rst_time;
 
+	struct vlan_group *vlgrp;
 	struct qlcnic_npar_info *npars;
 	struct qlcnic_eswitch *eswitch;
 	struct qlcnic_nic_template *nic_ops;
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 26a7d6b..10cebb1 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1380,24 +1380,28 @@ static struct sk_buff *qlcnic_process_rxbuf(struct qlcnic_adapter *adapter,
 }
 
 static int
-qlcnic_check_rx_tagging(struct qlcnic_adapter *adapter, struct sk_buff *skb)
+qlcnic_check_rx_tagging(struct qlcnic_adapter *adapter, struct sk_buff *skb,
+			u16 *vlan_tag)
 {
-	u16 vlan_tag;
 	struct ethhdr *eth_hdr;
 
-	if (!__vlan_get_tag(skb, &vlan_tag)) {
-		if (vlan_tag == adapter->pvid) {
-			/* strip the tag from the packet and send it up */
-			eth_hdr = (struct ethhdr *) skb->data;
-			memmove(skb->data + VLAN_HLEN, eth_hdr, ETH_ALEN * 2);
-			skb_pull(skb, VLAN_HLEN);
-			return 0;
-		}
+	if (!__vlan_get_tag(skb, vlan_tag)) {
+		eth_hdr = (struct ethhdr *) skb->data;
+		memmove(skb->data + VLAN_HLEN, eth_hdr, ETH_ALEN * 2);
+		skb_pull(skb, VLAN_HLEN);
+	}
+	if (!adapter->pvid)
+		return ...
[ message continues ]

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>


--

[ message continues ]

Amit, I noticed following bug in qlnic driver.

Also, skb->truesize should not be changed by drivers, unless dealing
with fragments.

When you have :
	skb->truesize = skb->len + sizeof(struct sk_buff);

you are lying to stack that can queue many "small" UDP packets,
accouting for small packets in socket rcvbuf, while the truesize was
really 1532 + sizeof(struct sk_buff)

Could you take a look ?

Thanks


[PATCH] qlcnic: dont assume NET_IP_ALIGN is 2

qlcnic driver allocates rx skbs and gives to hardware too bytes of extra
storage, allowing for corruption of kernel data.

NET_IP_ALIGN being 0 on some platforms (including x86), drivers should
not assume it's 2.

rds_ring->skb_size = rds_ring->dma_size + NET_IP_ALIGN;
...
skb = dev_alloc_skb(rds_ring->skb_size);
skb_reserve(skb, 2);
pci_map_single(pdev, skb->data, rds_ring->dma_size, PCI_DMA_FROMDEVICE);

(and rds_ring->skb_size == rds_ring->dma_size) -> bug


Because of extra alignment (1500 + 32) -> four extra bytes are available
before the struct skb_shared_info, so corruption is not noticed.

Note: this driver could use netdev_alloc_skb_ip_align()

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index 75ba744..60ab753 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1316,7 +1316,7 @@ qlcnic_alloc_rx_skb(struct qlcnic_adapter *adapter,
 		return -ENOMEM;
 	}
 
-	skb_reserve(skb, 2);
+	skb_reserve(skb, NET_IP_ALIGN);
 
 	dma = pci_map_single(pdev, skb->data,
 			rds_ring->dma_size, PCI_DMA_FROMDEVICE);


--

[ message continues ]

From: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-2.6, thanks Eric.
--

[ message continues ]

truesize accounts for the real size of buffers, not the used part in
them.

IMHO, a driver not dealing with fragments should not touch skb->truesize

# grep truesize drivers/net/tg3.c
<nothing>

If a driver deals with fragments, it probably should use "+=" operator
only, not hardcoding sizeof(struct sk_buff) thing that only core network
has to deal with.

$ find drivers/net/bnx2x|xargs grep truesize
drivers/net/bnx2x/bnx2x_cmn.c:		skb->truesize += frag_len;

Almost all drivers are fine, they are some of them that should be
changed.



--

[ message continues ]

I suspect following patch should be fine :

[PATCH net-next-2.6] qlnic: dont set skb->truesize

skb->truesize is set in core network.

Dont change it unless dealing with fragments.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/qlcnic/qlcnic_init.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/qlcnic/qlcnic_init.c b/drivers/net/qlcnic/qlcnic_init.c
index e26fa95..16dd9eb 100644
--- a/drivers/net/qlcnic/qlcnic_init.c
+++ b/drivers/net/qlcnic/qlcnic_init.c
@@ -1418,8 +1418,6 @@ qlcnic_process_rcv(struct qlcnic_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
-
 	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
 		adapter->stats.rxdropped++;
 		dev_kfree_skb(skb);
@@ -1491,8 +1489,6 @@ qlcnic_process_lro(struct qlcnic_adapter *adapter,
 
 	skb_put(skb, lro_length + data_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff) + skb_headroom(skb);
-
 	skb_pull(skb, l2_hdr_offset);
 
 	if (unlikely(qlcnic_check_rx_tagging(adapter, skb, &vid))) {
@@ -1732,8 +1728,6 @@ qlcnic_process_rcv_diag(struct qlcnic_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
-
 	if (!qlcnic_check_loopback_buff(skb->data))
 		adapter->diag_cnt++;
 


--

[ message continues ]

From: Eric Dumazet <eric.dumazet@gmail.com>

Looks good to me too, applied thanks!
--

[ message continues ]

From: Amit Salecha <amit.salecha@qlogic.com>

No, it will "fix" accounting.

You must charge to the SKB all of the non-shared memory that was
allocated to the SKB.

This means even if the packet only uses 128 bytes of the SKB
data area, you must still account for the full blob of linear
memory that was allocated for the SKB data area in skb->truesize.

Otherwise remote attackers could consume enormous amounts of memory by
tricking our socket accounting via carefully sized packets.
--

[ message continues ]

Wont this affect throughput ?
As problem discuss in this thread http://www.mail-archive.com/netdev@vger.kernel.org/msg06848.html, it can affect tcp window scaling.

-Amit
--

[ message continues ]

Amit, if you believe this is a problem, you should address it for all
NICS, not only qlcnic.

Qlcnic was lying to stack, because it consumed 2Kbytes blocs and
pretended they were consuming skb->len bytes.
(assuming MTU=1500, problem is worse if MTU is bigger)

So in order to improve "throughput", you were allowing for memory
exhaust and freeze of the _machine_ ?



--

[ message continues ]

You must have big machines in your lab and never hit OOM ?

You really should take a look on various files in net/core, net/ipv4
trees. And files like "/proc/sys/net/tcp_mem", "/proc/sys/net/udp_mem"

In fact, truesize is _underestimated_ : (we dont account for struct
skb_shared_info) and kmalloc() rounding

We probably should use this patch (without having to check all possible
net drivers !)

Problem is this would slow down alloc_skb(), so this patch is not for
inclusion.

cheap alternative would be to use 

size + sizeof(struct sk_buff) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info))

If you think about it, when 128bit arches come, truesize will grow anyway.
If some tuning is needed in our stack, we'll do it.

(socket api SO_RCVBUF/ SO_SNDBUF is the problem, because
 applications are not aware of packetization or kernel internals)

SOCK_MIN_RCVBUF is way too small, since sizeof(struct sk_buff) 
is already close to 256. I guess we cannot even receive a single frame.

 include/net/sock.h |    2 +-
 net/core/skbuff.c  |    2 +-
 net/core/sock.c    |    8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)


diff --git a/include/net/sock.h b/include/net/sock.h
index 8ae97c4..348fc9e 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1558,7 +1558,7 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
 }
 
 #define SOCK_MIN_SNDBUF 2048
-#define SOCK_MIN_RCVBUF 256
+#define SOCK_MIN_RCVBUF 1024
 
 static inline void sk_stream_moderate_sndbuf(struct sock *sk)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 752c197..5ab2e8e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -196,7 +196,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	 * the tail pointer in struct sk_buff!
 	 */
 	memset(skb, 0, offsetof(struct sk_buff, tail));
-	skb->truesize = size + sizeof(struct sk_buff);
+	skb->truesize = ksize(data) + sizeof(struct sk_buff);
 	atomic_set(&skb->users, 1);
 	skb->head = data;
 ...
[ message continues ]

From: Amit Salecha <amit.salecha@qlogic.com>

Yes, it will.

Do you understand that we enforce both socket-level and system-wide
networking buffer usage in the stack?  And this limiting is based
upon skb->truesize and therefore only works if skb->truesize is
accurate?

It's meant to keep people from attacking a server and consuming large
percentages of system memory with networking buffer memory such that
other tasks cannot complete successfully.

And by mis-reporting the truesize you are subverting that entirely.

This qlcnic truesize bug is a huge security hole, can't you see this
now?

--

[ message continues ]

BTW, drivers/net/netxen/netxen_nic_init.c has same problem.

[PATCH] netxen: dont set skb->truesize

skb->truesize is set in core network.

Dont change it unless dealing with fragments.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/netxen/netxen_nic_init.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/netxen/netxen_nic_init.c b/drivers/net/netxen/netxen_nic_init.c
index cabae7b..b075a35 100644
--- a/drivers/net/netxen/netxen_nic_init.c
+++ b/drivers/net/netxen/netxen_nic_init.c
@@ -1540,7 +1540,6 @@ netxen_process_rcv(struct netxen_adapter *adapter,
 	if (pkt_offset)
 		skb_pull(skb, pkt_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff);
 	skb->protocol = eth_type_trans(skb, netdev);
 
 	napi_gro_receive(&sds_ring->napi, skb);
@@ -1602,8 +1601,6 @@ netxen_process_lro(struct netxen_adapter *adapter,
 
 	skb_put(skb, lro_length + data_offset);
 
-	skb->truesize = skb->len + sizeof(struct sk_buff) + skb_headroom(skb);
-
 	skb_pull(skb, l2_hdr_offset);
 	skb->protocol = eth_type_trans(skb, netdev);
 


--

[ message continues ]

From: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-2.6, and I backported the qlcnic patch in net-next-2.6
into net-2.6 too.

Thanks.
--

[ message continues ]

From: Amit Kumar Salecha <amit.salecha@qlogic.com>

All applied, thanks.
--

[ message continues ]