login
Search
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 --
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 --
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
--
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 ...Acked-by: Eric Dumazet <eric.dumazet@gmail.com> --
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); --
From: Eric Dumazet <eric.dumazet@gmail.com> Applied to net-2.6, thanks Eric. --
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. --
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++;
--
From: Eric Dumazet <eric.dumazet@gmail.com> Looks good to me too, applied thanks! --
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. --
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 --
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_ ? --
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;
...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? --
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); --
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. --
From: Amit Kumar Salecha <amit.salecha@qlogic.com> All applied, thanks. --
