* [PATCH] stmmac: priv->lock can be used uninitialized
From: Vlad Lungu @ 2010-11-30 8:52 UTC (permalink / raw)
To: netdev; +Cc: peppe.cavallaro, davem, Vlad Lungu
To reproduce: if connman (http://connman.net/) is started,
inserting the stmmac module triggers a "BUG: spinlock bad magic on CPU#0".
Registering the device in stmmac_probe() sends a notification to connman
which brings the interface up before the lock is initialized.
Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
---
drivers/net/stmmac/stmmac_main.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
index 730a6fd..e960501 100644
--- a/drivers/net/stmmac/stmmac_main.c
+++ b/drivers/net/stmmac/stmmac_main.c
@@ -1516,6 +1516,8 @@ static int stmmac_probe(struct net_device *dev)
pr_warning("\tno valid MAC address;"
"please, use ifconfig or nwhwconfig!\n");
+ spin_lock_init(&priv->lock);
+
ret = register_netdev(dev);
if (ret) {
pr_err("%s: ERROR %i registering the device\n",
@@ -1527,8 +1529,6 @@ static int stmmac_probe(struct net_device *dev)
dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
(dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
- spin_lock_init(&priv->lock);
-
return ret;
}
--
1.6.0.2
^ permalink raw reply related
* Re: multi bpf filter will impact performance?
From: Eric Dumazet @ 2010-11-30 11:17 UTC (permalink / raw)
To: Rui; +Cc: netdev
In-Reply-To: <AANLkTikx68M43+vv+Rav_HCJMJnuc15TtuBgmbv2xP=U@mail.gmail.com>
Le mardi 30 novembre 2010 à 17:22 +0800, Rui a écrit :
> PCAP_FRAMES=32000 ./tcpdump_MMAP -i eth4 'udp dst port 2152 and (
> (((ether[48:1]&0x07)>0) and
> (((ether[66:1]+ether[67:1]+ether[68:1]+ether[69:1]+ether[70:1]+ether[71:1]+ether[72:1]+ether[73:1])&0x03)==0))
> or (((ether[48:1]&0x07)==0) and
> (((ether[62:1]+ether[63:1]+ether[64:1]+ether[65:1]+ether[66:1]+ether[67:1]+ether[68:1]+ether[69:1])&0x03)==0))
> ) ' -w /dev/null -s 4096 2>f1.log &
(000) ldh [12]
(001) jeq #0x86dd jt 2 jf 6
(002) ldb [20]
(003) jeq #0x11 jt 4 jf 95
(004) ldh [56]
(005) jeq #0x868 jt 14 jf 95
(006) jeq #0x800 jt 7 jf 95
(007) ldb [23]
(008) jeq #0x11 jt 9 jf 95
(009) ldh [20]
(010) jset #0x1fff jt 95 jf 11
(011) ldxb 4*([14]&0xf)
(012) ldh [x + 16]
(013) jeq #0x868 jt 14 jf 95
(014) ldb [48]
(015) and #0x7
(016) ldx #0x0
(017) jgt x jt 18 jf 55
(018) ldb [66]
(019) st M[4]
(020) ldb [67]
(021) tax
(022) ld M[4]
(023) add x
(024) st M[6]
(025) ldb [68]
(026) tax
(027) ld M[6]
(028) add x
(029) st M[8]
(030) ldb [69]
(031) tax
(032) ld M[8]
(033) add x
(034) st M[10]
(035) ldb [70]
(036) tax
(037) ld M[10]
(038) add x
(039) st M[12]
(040) ldb [71]
(041) tax
(042) ld M[12]
(043) add x
(044) st M[14]
(045) ldb [72]
(046) tax
(047) ld M[14]
(048) add x
(049) st M[0]
(050) ldb [73]
(051) tax
(052) ld M[0]
(053) add x
[deleted part]
Ouch... we miss a "add{bh } [byteoff]" instruction, or "ldx{bh } [byteoff]"
ldb [66]
ldxb [67]
add x
ldxb [68]
add x
ldxb [69]
add x
ldxb [70]
add x
ldxb [71]
add x
ldxb [72]
add x
ldxb [73]
add x
...
With current instruction set, pcap optimizer could at least do something like :
ldb [66]
tax
ldb [67]
add x
tax
ldb [68]
add x
tax
ldb [69]
add x
tax
ldb [70]
add x
tax
ldb [71]
add x
tax
ldb [72]
add x
tax
ldb [73]
add x
...
^ permalink raw reply
* Re: [PATCH net-next-2.6 v6 06/20] can: EG20T PCH: Fix endianness issue
From: Marc Kleine-Budde @ 2010-11-30 12:05 UTC (permalink / raw)
To: Tomoya MORINAGA
Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w, Samuel Ortiz,
margie.foster-ral2JQCrhuEAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
yong.y.wang-ral2JQCrhuEAvxtiuMwx3w,
kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w, Wolfgang Grandegger,
joel.clark-ral2JQCrhuEAvxtiuMwx3w, David S. Miller,
Christian Pellegrin, qi.wang-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <4CF47BAA.2090209-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 8922 bytes --]
On 11/30/2010 05:20 AM, Tomoya MORINAGA wrote:
> there is endianness issue both Tx and Rx.
> Currently, data is set like below.
> Register:
> MSB--LSB
> x x D0 D1
> x x D2 D3
> x x D4 D5
> x x D6 D7
>
> But Data to be sent must be set like below.
> Register:
> MSB--LSB
> x x D1 D0
> x x D3 D2
> x x D5 D4
> x x D7 D6 (x means reserved area.)
The patch does more than fixing the endianess issue. Please split non
endianess related changes into seperate patch.
regards, Marc
>
> Signed-off-by: Tomoya MORINAGA <tomoya-linux-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
> ---
> drivers/net/can/pch_can.c | 118 +++++++++++++++++++++------------------------
> 1 files changed, 55 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index 6437e60..98e7e9f 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
> @@ -134,10 +134,7 @@ struct pch_can_if_regs {
> u32 id1;
> u32 id2;
> u32 mcont;
> - u32 dataa1;
> - u32 dataa2;
> - u32 datab1;
> - u32 datab2;
> + u32 data[4];./internal/TsiStandard/appl/gendata/desc.c
> u32 rsv[13];
> };
>
> @@ -420,10 +417,10 @@ static void pch_can_clear_buffers(struct pch_can_priv *priv)
> iowrite32(0x0, &priv->regs->ifregs[0].id1);
> iowrite32(0x0, &priv->regs->ifregs[0].id2);
> iowrite32(0x0, &priv->regs->ifregs[0].mcont);
> - iowrite32(0x0, &priv->regs->ifregs[0].dataa1);
> - iowrite32(0x0, &priv->regs->ifregs[0].dataa2);
> - iowrite32(0x0, &priv->regs->ifregs[0].datab1);
> - iowrite32(0x0, &priv->regs->ifregs[0].datab2);
> + iowrite32(0x0, &priv->regs->ifregs[0].data[0]);
> + iowrite32(0x0, &priv->regs->ifregs[0].data[1]);
> + iowrite32(0x0, &priv->regs->ifregs[0].data[2]);
> + iowrite32(0x0, &priv->regs->ifregs[0].data[3]);
> iowrite32(PCH_CMASK_RDWR | PCH_CMASK_MASK |
> PCH_CMASK_ARB | PCH_CMASK_CTRL,
> &priv->regs->ifregs[0].cmask);
> @@ -437,10 +434,10 @@ static void pch_can_clear_buffers(struct pch_can_priv *priv)
> iowrite32(0x0, &priv->regs->ifregs[1].id1);
> iowrite32(0x0, &priv->regs->ifregs[1].id2);
> iowrite32(0x0, &priv->regs->ifregs[1].mcont);
> - iowrite32(0x0, &priv->regs->ifregs[1].dataa1);
> - iowrite32(0x0, &priv->regs->ifregs[1].dataa2);
> - iowrite32(0x0, &priv->regs->ifregs[1].datab1);
> - iowrite32(0x0, &priv->regs->ifregs[1].datab2);
> + iowrite32(0x0, &priv->regs->ifregs[1].data[0]);
> + iowrite32(0x0, &priv->regs->ifregs[1].data[1]);
> + iowrite32(0x0, &priv->regs->ifregs[1].data[2]);
> + iowrite32(0x0, &priv->regs->ifregs[1].data[3]);
> iowrite32(PCH_CMASK_RDWR | PCH_CMASK_MASK |
> PCH_CMASK_ARB | PCH_CMASK_CTRL,
> &priv->regs->ifregs[1].cmask);
> @@ -703,12 +700,13 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 int_stat)
> canid_t id;
> u32 ide;
> u32 rtr;
> - int i, j, k;
> + int i, k;
> int rcv_pkts = 0;
> struct sk_buff *skb;
> struct can_frame *cf;
> struct pch_can_priv *priv = netdev_priv(ndev);
> struct net_device_stats *stats = &(priv->ndev->stats);
> + u16 data_reg;
>
> /* Reading the messsage object from the Message RAM */
> iowrite32(PCH_CMASK_RX_TX_GET, &priv->regs->ifregs[0].cmask);
> @@ -774,12 +772,10 @@ static int pch_can_rx_normal(struct net_device *ndev, u32 int_stat)
> ((ioread32(&priv->regs->ifregs[0].mcont)) & 0x0f);
> }
>
> - for (i = 0, j = 0; i < cf->can_dlc; j++) {
> - reg = ioread32(&priv->regs->ifregs[0].dataa1 + j*4);
> - cf->data[i++] = cpu_to_le32(reg & 0xff);
> - if (i == cf->can_dlc)
> - break;
> - cf->data[i++] = cpu_to_le32((reg >> 8) & 0xff);
> + for (i = 0; i < cf->can_dlc; i += 2) {
> + data_reg = ioread16(&priv->regs->ifregs[0].data[i / 2]);
> + cf->data[i] = data_reg & 0xff;
> + cf->data[i + 1] = data_reg >> 8;
this is endianess related....good
> }
>
> netif_receive_skb(skb);
> @@ -815,72 +811,71 @@ RX_NEXT:
>
> return rcv_pkts;
> }
> -static int pch_can_rx_poll(struct napi_struct *napi, int quota)
> +
> +static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
> {
> - struct net_device *ndev = napi->dev;
> struct pch_can_priv *priv = netdev_priv(ndev);
> struct net_device_stats *stats = &(priv->ndev->stats);
> u32 dlc;
> +
> + can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
> + iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
> + &priv->regs->ifregs[1].cmask);
> + pch_can_check_if_busy(&priv->regs->ifregs[1].creq, int_stat);
> + dlc = get_can_dlc(ioread32(&priv->regs->ifregs[1].mcont) &
> + PCH_IF_MCONT_DLC);
> + stats->tx_bytes += dlc;
> + stats->tx_packets++;
> + if (int_stat == PCH_TX_OBJ_END)
> + netif_wake_queue(ndev);
> +}
but this is something else...please split into a seperate patch.
> +
> +static int pch_can_rx_poll(struct napi_struct *napi, int quota)
> +{
> + struct net_device *ndev = napi->dev;
> + struct pch_can_priv *priv = netdev_priv(ndev);
> u32 int_stat;
> int rcv_pkts = 0;
> u32 reg_stat;
>
> int_stat = pch_can_int_pending(priv);
> if (!int_stat)
> - return 0;
> + goto end;
>
> -INT_STAT:
> - if (int_stat == PCH_STATUS_INT) {
> + if ((int_stat == PCH_STATUS_INT) && (quota > 0)) {
> reg_stat = ioread32(&priv->regs->stat);
> if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL)) {
> - if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL)
> + if (reg_stat & PCH_BUS_OFF ||
> + (reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) {
> pch_can_error(ndev, reg_stat);
> + quota--;
> + }
dito
> }
>
> - if (reg_stat & PCH_TX_OK) {
> - iowrite32(PCH_CMASK_RX_TX_GET,
> - &priv->regs->ifregs[1].cmask);
> - pch_can_check_if_busy(&priv->regs->ifregs[1].creq,
> - ioread32(&priv->regs->intr));
> + if (reg_stat & PCH_TX_OK)
> pch_can_bit_clear(&priv->regs->stat, PCH_TX_OK);
> - }
dito
>
> if (reg_stat & PCH_RX_OK)
> pch_can_bit_clear(&priv->regs->stat, PCH_RX_OK);
>
> int_stat = pch_can_int_pending(priv);
> - if (int_stat == PCH_STATUS_INT)
> - goto INT_STAT;
> }
>
> -MSG_OBJ:
> + if (quota == 0)
> + goto end;
> +
> if ((int_stat >= PCH_RX_OBJ_START) && (int_stat <= PCH_RX_OBJ_END)) {
> - rcv_pkts = pch_can_rx_normal(ndev, int_stat);
> - if (rcv_pkts < 0)
> - return 0;
> + rcv_pkts += pch_can_rx_normal(ndev, int_stat);
> + quota -= rcv_pkts;
> + if (quota < 0)
> + goto end;
same here
> } else if ((int_stat >= PCH_TX_OBJ_START) &&
> (int_stat <= PCH_TX_OBJ_END)) {
> /* Handle transmission interrupt */
> - can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
> - iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
> - &priv->regs->ifregs[1].cmask);
> - dlc = ioread32(&priv->regs->ifregs[1].mcont) &
> - PCH_IF_MCONT_DLC;
> - pch_can_check_if_busy(&priv->regs->ifregs[1].creq, int_stat);
> - if (dlc > 8)
> - dlc = 8;
> - stats->tx_bytes += dlc;
> - stats->tx_packets++;
> - if (int_stat == PCH_TX_OBJ_END)
> - netif_wake_queue(ndev);
> + pch_can_tx_complete(ndev, int_stat);
same here
> }
>
> - int_stat = pch_can_int_pending(priv);
> - if (int_stat == PCH_STATUS_INT)
> - goto INT_STAT;
> - else if (int_stat >= 1 && int_stat <= 32)
> - goto MSG_OBJ;
> -
> +end:
> napi_complete(napi);
> pch_can_set_int_enables(priv, PCH_CAN_ALL);
>
> @@ -1013,10 +1008,10 @@ static int pch_close(struct net_device *ndev)
>
> static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
> {
> - int i, j;
> struct pch_can_priv *priv = netdev_priv(ndev);
> struct can_frame *cf = (struct can_frame *)skb->data;
> int tx_buffer_avail = 0;
> + int i;
>
> if (can_dropped_invalid_skb(ndev, skb))
> return NETDEV_TX_OK;
> @@ -1057,13 +1052,10 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
> if (cf->can_id & CAN_RTR_FLAG)
> pch_can_bit_clear(&priv->regs->ifregs[1].id2, PCH_ID2_DIR);
>
> - for (i = 0, j = 0; i < cf->can_dlc; j++) {
> - iowrite32(le32_to_cpu(cf->data[i++]),
> - (&priv->regs->ifregs[1].dataa1) + j*4);
> - if (i == cf->can_dlc)
> - break;
> - iowrite32(le32_to_cpu(cf->data[i++] << 8),
> - (&priv->regs->ifregs[1].dataa1) + j*4);
> + /* Copy data to register */
> + for (i = 0; i < cf->can_dlc; i += 2) {
> + iowrite16(cf->data[i] | (cf->data[i + 1] << 8),
> + &priv->regs->ifregs[1].data[i / 2]);
good.
> }
>
> can_put_echo_skb(skb, ndev, tx_buffer_avail - PCH_RX_OBJ_END - 1);
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
[-- Attachment #2: Type: text/plain, Size: 188 bytes --]
_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core
^ permalink raw reply
* [PATCH] Add missing lockdep class names for AF_ALG
From: Miloslav Trmač @ 2010-11-30 13:15 UTC (permalink / raw)
To: herbert, davem; +Cc: netdev, linux-crypto, Miloslav Trmač
---
net/core/sock.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index 3eed542..634d5bc 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -157,7 +157,7 @@ static const char *const af_family_key_strings[AF_MAX+1] = {
"sk_lock-27" , "sk_lock-28" , "sk_lock-AF_CAN" ,
"sk_lock-AF_TIPC" , "sk_lock-AF_BLUETOOTH", "sk_lock-IUCV" ,
"sk_lock-AF_RXRPC" , "sk_lock-AF_ISDN" , "sk_lock-AF_PHONET" ,
- "sk_lock-AF_IEEE802154", "sk_lock-AF_CAIF" ,
+ "sk_lock-AF_IEEE802154", "sk_lock-AF_CAIF" , "sk_lock-AF_ALG" ,
"sk_lock-AF_MAX"
};
static const char *const af_family_slock_key_strings[AF_MAX+1] = {
@@ -173,7 +173,7 @@ static const char *const af_family_slock_key_strings[AF_MAX+1] = {
"slock-27" , "slock-28" , "slock-AF_CAN" ,
"slock-AF_TIPC" , "slock-AF_BLUETOOTH", "slock-AF_IUCV" ,
"slock-AF_RXRPC" , "slock-AF_ISDN" , "slock-AF_PHONET" ,
- "slock-AF_IEEE802154", "slock-AF_CAIF" ,
+ "slock-AF_IEEE802154", "slock-AF_CAIF" , "slock-AF_ALG" ,
"slock-AF_MAX"
};
static const char *const af_family_clock_key_strings[AF_MAX+1] = {
@@ -189,7 +189,7 @@ static const char *const af_family_clock_key_strings[AF_MAX+1] = {
"clock-27" , "clock-28" , "clock-AF_CAN" ,
"clock-AF_TIPC" , "clock-AF_BLUETOOTH", "clock-AF_IUCV" ,
"clock-AF_RXRPC" , "clock-AF_ISDN" , "clock-AF_PHONET" ,
- "clock-AF_IEEE802154", "clock-AF_CAIF" ,
+ "clock-AF_IEEE802154", "clock-AF_CAIF" , "clock-AF_ALG" ,
"clock-AF_MAX"
};
--
1.7.3.2
^ permalink raw reply related
* [PATCH] fix hang in dmfe driver on sending of big packet (linux-2.6.35)
From: Alexander V. Lukyanov @ 2010-11-30 13:46 UTC (permalink / raw)
To: netdev, tori
[-- Attachment #1: Type: text/plain, Size: 223 bytes --]
Hello!
This patch fixes hang in dmfe driver on attempt of sending a big packet.
Without this patch the code stops the queue and never wakes it again.
Signed-off-by: Alexander V. Lukyanov <lav@netis.ru>
--
Alexander.
[-- Attachment #2: dmfe.patch --]
[-- Type: text/plain, Size: 602 bytes --]
--- dmfe.c.1 2010-11-30 16:21:52.758465207 +0300
+++ dmfe.c 2010-11-30 16:24:45.301468601 +0300
@@ -687,18 +687,18 @@
unsigned long flags;
DMFE_DBUG(0, "dmfe_start_xmit", 0);
- /* Resource flag check */
- netif_stop_queue(dev);
-
/* Too large packet check */
if (skb->len > MAX_PACKET_SIZE) {
pr_err("big packet = %d\n", (u16)skb->len);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}
+ /* Resource flag check */
+ netif_stop_queue(dev);
+
spin_lock_irqsave(&db->lock, flags);
/* No Tx resource check, it never happen nromally */
if (db->tx_queue_cnt >= TX_FREE_DESC_CNT) {
^ permalink raw reply
* Bonding, GRO and tcp_reordering
From: Simon Horman @ 2010-11-30 13:55 UTC (permalink / raw)
To: netdev
Hi,
I just wanted to share what is a rather pleasing,
though to me somewhat surprising result.
I am testing bonding using balance-rr mode with three physical links to try
to get > gigabit speed for a single stream. Why? Because I'd like to run
various tests at > gigabit speed and I don't have any 10G hardware at my
disposal.
The result I have is that with a 1500 byte MTU, tcp_reordering=3 and both
LSO and GSO disabled on both the sender and receiver I see:
# netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 1472
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216
(172.17.60.216) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
87380 16384 1472 10.01 1646.13 40.01 -1.00 3.982 -1.000
But with GRO enabled on the receiver I see.
# netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 1472
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216
(172.17.60.216) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
87380 16384 1472 10.01 2613.83 19.32 -1.00 1.211 -1.000
Which is much better than any result I get tweaking tcp_reordering when
GRO is disabled on the receiver.
Tweaking tcp_reordering when GRO is enabled on the receiver seems to have
negligible effect. Which is interesting, because my brief reading on the
subject indicated that tcp_reordering was the key tuning parameter for
bonding with balance-rr.
The only other parameter that seemed to have significant effect was to
increase the mtu. In the case of MTU=9000, GRO seemed to have a negative
impact on throughput, though a significant positive effect on CPU
utilisation.
MTU=9000, sender,receiver:tcp_reordering=3(default), receiver:GRO=off
netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 9872
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
87380 16384 9872 10.01 2957.52 14.89 -1.00 0.825 -1.000
MTU=9000, sender,receiver:tcp_reordering=3(default), receiver:GRO=on
netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 9872
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
87380 16384 9872 10.01 2847.64 10.84 -1.00 0.624 -1.000
Test run using 2.6.37-rc1
^ permalink raw reply
* [PATCH 1/2] af_packet: use vmalloc_to_page() instead for the addresss returned by vmalloc()
From: Changli Gao @ 2010-11-30 13:56 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Dumazet, Jiri Pirko, Neil Horman, netdev, Changli Gao
The following commit causes the pgv->buffer may point to the memory
returned by vmalloc(). And we can't use virt_to_page() for the vmalloc
address.
This patch introduces a new inline function pgv_to_page(), which calls
vmalloc_to_page() for the vmalloc address, and virt_to_page() for the
__get_free_pages address.
commit 0e3125c755445664f00ad036e4fc2cd32fd52877
Author: Neil Horman <nhorman@tuxdriver.com>
Date: Tue Nov 16 10:26:47 2010 -0800
packet: Enhance AF_PACKET implementation to not require high order contiguous memory allocation (v4)
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
net/packet/af_packet.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 422705d..0171b20 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -224,6 +224,13 @@ struct packet_skb_cb {
#define PACKET_SKB_CB(__skb) ((struct packet_skb_cb *)((__skb)->cb))
+static inline struct page *pgv_to_page(void *addr)
+{
+ if (is_vmalloc_addr(addr))
+ return vmalloc_to_page(addr);
+ return virt_to_page(addr);
+}
+
static void __packet_set_status(struct packet_sock *po, void *frame, int status)
{
union {
@@ -236,11 +243,11 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
switch (po->tp_version) {
case TPACKET_V1:
h.h1->tp_status = status;
- flush_dcache_page(virt_to_page(&h.h1->tp_status));
+ flush_dcache_page(pgv_to_page(&h.h1->tp_status));
break;
case TPACKET_V2:
h.h2->tp_status = status;
- flush_dcache_page(virt_to_page(&h.h2->tp_status));
+ flush_dcache_page(pgv_to_page(&h.h2->tp_status));
break;
default:
pr_err("TPACKET version not supported\n");
@@ -263,10 +270,10 @@ static int __packet_get_status(struct packet_sock *po, void *frame)
h.raw = frame;
switch (po->tp_version) {
case TPACKET_V1:
- flush_dcache_page(virt_to_page(&h.h1->tp_status));
+ flush_dcache_page(pgv_to_page(&h.h1->tp_status));
return h.h1->tp_status;
case TPACKET_V2:
- flush_dcache_page(virt_to_page(&h.h2->tp_status));
+ flush_dcache_page(pgv_to_page(&h.h2->tp_status));
return h.h2->tp_status;
default:
pr_err("TPACKET version not supported\n");
@@ -803,8 +810,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
struct page *p_start, *p_end;
u8 *h_end = h.raw + macoff + snaplen - 1;
- p_start = virt_to_page(h.raw);
- p_end = virt_to_page(h_end);
+ p_start = pgv_to_page(h.raw);
+ p_end = pgv_to_page(h_end);
while (p_start <= p_end) {
flush_dcache_page(p_start);
p_start++;
@@ -915,7 +922,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
}
err = -EFAULT;
- page = virt_to_page(data);
+ page = pgv_to_page(data);
offset = offset_in_page(data);
len_max = PAGE_SIZE - offset;
len = ((to_write > len_max) ? len_max : to_write);
^ permalink raw reply related
* Re: [PATCH] fix hang in dmfe driver on sending of big packet (linux-2.6.35)
From: Eric Dumazet @ 2010-11-30 13:57 UTC (permalink / raw)
To: Alexander V. Lukyanov; +Cc: netdev, tori
In-Reply-To: <20101130134626.GA1856@lw.yar.ru>
From: Alexander V. Lukyanov <lav@netis.ru>
Le mardi 30 novembre 2010 à 16:46 +0300, Alexander V. Lukyanov a écrit :
> Hello!
>
> This patch fixes hang in dmfe driver on attempt of sending a big packet.
> Without this patch the code stops the queue and never wakes it again.
>
> Signed-off-by: Alexander V. Lukyanov <lav@netis.ru>
>
Nice catch, but your patch is not a "diff -p1" one
I did it for net-2.6 tree :
Thanks
[PATCH] tulip: fix hang in dmfe driver on sending of big packet
This patch fixes hang in dmfe driver on attempt of sending a big packet.
Without this patch the code stops the queue and never wakes it again.
Signed-off-by: Alexander V. Lukyanov <lav@netis.ru>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
---
drivers/net/tulip/dmfe.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/tulip/dmfe.c b/drivers/net/tulip/dmfe.c
index a9f7d5d..7064e03 100644
--- a/drivers/net/tulip/dmfe.c
+++ b/drivers/net/tulip/dmfe.c
@@ -688,9 +688,6 @@ static netdev_tx_t dmfe_start_xmit(struct sk_buff *skb,
DMFE_DBUG(0, "dmfe_start_xmit", 0);
- /* Resource flag check */
- netif_stop_queue(dev);
-
/* Too large packet check */
if (skb->len > MAX_PACKET_SIZE) {
pr_err("big packet = %d\n", (u16)skb->len);
@@ -698,6 +695,9 @@ static netdev_tx_t dmfe_start_xmit(struct sk_buff *skb,
return NETDEV_TX_OK;
}
+ /* Resource flag check */
+ netif_stop_queue(dev);
+
spin_lock_irqsave(&db->lock, flags);
/* No Tx resource check, it never happen nromally */
^ permalink raw reply related
* [PATCH 2/2] af_packet: replace struct pgv with char **
From: Changli Gao @ 2010-11-30 13:57 UTC (permalink / raw)
To: David S. Miller
Cc: Eric Dumazet, Jiri Pirko, Neil Horman, netdev, Changli Gao
As we can check if an address is vmalloc address with is_vmalloc_addr(),
we can replace struct pgv with char **. Then we may get more pg_vecs.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
net/packet/af_packet.c | 50 ++++++++++++++++---------------------------------
1 file changed, 17 insertions(+), 33 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0171b20..a26f981 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -164,14 +164,8 @@ struct packet_mreq_max {
static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
int closing, int tx_ring);
-#define PGV_FROM_VMALLOC 1
-struct pgv {
- char *buffer;
- unsigned char flags;
-};
-
struct packet_ring_buffer {
- struct pgv *pg_vec;
+ char **pg_vec;
unsigned int head;
unsigned int frames_per_block;
unsigned int frame_size;
@@ -297,8 +291,7 @@ static void *packet_lookup_frame(struct packet_sock *po,
pg_vec_pos = position / rb->frames_per_block;
frame_offset = position % rb->frames_per_block;
- h.raw = rb->pg_vec[pg_vec_pos].buffer +
- (frame_offset * rb->frame_size);
+ h.raw = rb->pg_vec[pg_vec_pos] + (frame_offset * rb->frame_size);
if (status != __packet_get_status(po, h.raw))
return NULL;
@@ -2340,26 +2333,24 @@ static const struct vm_operations_struct packet_mmap_ops = {
.close = packet_mm_close,
};
-static void free_pg_vec(struct pgv *pg_vec, unsigned int order,
- unsigned int len)
+static void free_pg_vec(char **pg_vec, unsigned int order, unsigned int len)
{
int i;
for (i = 0; i < len; i++) {
- if (likely(pg_vec[i].buffer)) {
- if (pg_vec[i].flags & PGV_FROM_VMALLOC)
- vfree(pg_vec[i].buffer);
+ if (likely(pg_vec[i])) {
+ if (is_vmalloc_addr(pg_vec[i]))
+ vfree(pg_vec[i]);
else
- free_pages((unsigned long)pg_vec[i].buffer,
+ free_pages((unsigned long)pg_vec[i],
order);
- pg_vec[i].buffer = NULL;
+ pg_vec[i] = NULL;
}
}
kfree(pg_vec);
}
-static inline char *alloc_one_pg_vec_page(unsigned long order,
- unsigned char *flags)
+static inline char *alloc_one_pg_vec_page(unsigned long order)
{
char *buffer = NULL;
gfp_t gfp_flags = GFP_KERNEL | __GFP_COMP |
@@ -2373,7 +2364,6 @@ static inline char *alloc_one_pg_vec_page(unsigned long order,
/*
* __get_free_pages failed, fall back to vmalloc
*/
- *flags |= PGV_FROM_VMALLOC;
buffer = vzalloc((1 << order) * PAGE_SIZE);
if (buffer)
@@ -2382,7 +2372,6 @@ static inline char *alloc_one_pg_vec_page(unsigned long order,
/*
* vmalloc failed, lets dig into swap here
*/
- *flags = 0;
gfp_flags &= ~__GFP_NORETRY;
buffer = (char *)__get_free_pages(gfp_flags, order);
if (buffer)
@@ -2394,20 +2383,19 @@ static inline char *alloc_one_pg_vec_page(unsigned long order,
return NULL;
}
-static struct pgv *alloc_pg_vec(struct tpacket_req *req, int order)
+static char **alloc_pg_vec(struct tpacket_req *req, int order)
{
unsigned int block_nr = req->tp_block_nr;
- struct pgv *pg_vec;
+ char **pg_vec;
int i;
- pg_vec = kcalloc(block_nr, sizeof(struct pgv), GFP_KERNEL);
+ pg_vec = kcalloc(block_nr, sizeof(char *), GFP_KERNEL);
if (unlikely(!pg_vec))
goto out;
for (i = 0; i < block_nr; i++) {
- pg_vec[i].buffer = alloc_one_pg_vec_page(order,
- &pg_vec[i].flags);
- if (unlikely(!pg_vec[i].buffer))
+ pg_vec[i] = alloc_one_pg_vec_page(order);
+ if (unlikely(!pg_vec[i]))
goto out_free_pgvec;
}
@@ -2424,7 +2412,7 @@ out_free_pgvec:
static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
int closing, int tx_ring)
{
- struct pgv *pg_vec = NULL;
+ char **pg_vec = NULL;
struct packet_sock *po = pkt_sk(sk);
int was_running, order = 0;
struct packet_ring_buffer *rb;
@@ -2587,16 +2575,12 @@ static int packet_mmap(struct file *file, struct socket *sock,
for (i = 0; i < rb->pg_vec_len; i++) {
struct page *page;
- void *kaddr = rb->pg_vec[i].buffer;
+ void *kaddr = rb->pg_vec[i];
int pg_num;
for (pg_num = 0; pg_num < rb->pg_vec_pages;
pg_num++) {
- if (rb->pg_vec[i].flags & PGV_FROM_VMALLOC)
- page = vmalloc_to_page(kaddr);
- else
- page = virt_to_page(kaddr);
-
+ page = pgv_to_page(kaddr);
err = vm_insert_page(vma, start, page);
if (unlikely(err))
goto out;
^ permalink raw reply related
* Re: [PATCH 1/2] af_packet: use vmalloc_to_page() instead for the addresss returned by vmalloc()
From: Eric Dumazet @ 2010-11-30 14:12 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, Jiri Pirko, Neil Horman, netdev
In-Reply-To: <1291125408-14389-1-git-send-email-xiaosuo@gmail.com>
Le mardi 30 novembre 2010 à 21:56 +0800, Changli Gao a écrit :
> The following commit causes the pgv->buffer may point to the memory
> returned by vmalloc(). And we can't use virt_to_page() for the vmalloc
> address.
>
> This patch introduces a new inline function pgv_to_page(), which calls
> vmalloc_to_page() for the vmalloc address, and virt_to_page() for the
> __get_free_pages address.
>
> commit 0e3125c755445664f00ad036e4fc2cd32fd52877
> Author: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue Nov 16 10:26:47 2010 -0800
>
> packet: Enhance AF_PACKET implementation to not require high order contiguous memory allocation (v4)
>
nice catch.
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> net/packet/af_packet.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 422705d..0171b20 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -224,6 +224,13 @@ struct packet_skb_cb {
>
> #define PACKET_SKB_CB(__skb) ((struct packet_skb_cb *)((__skb)->cb))
>
> +static inline struct page *pgv_to_page(void *addr)
> +{
> + if (is_vmalloc_addr(addr))
> + return vmalloc_to_page(addr);
Hmm, I am wondering if calling vmalloc_to_page(addr) several times for
each packet is not too expensive ? I believe it is.
What about caching "struct page *" pointer somewhere ?
Then later we have :
> - p_start = virt_to_page(h.raw);
> - p_end = virt_to_page(h_end);
> + p_start = pgv_to_page(h.raw);
> + p_end = pgv_to_page(h_end);
> while (p_start <= p_end) {
> flush_dcache_page(p_start);
> p_start++;
This was OK before Neil patch... after vmalloc(), assumption that
p_start can be incremented is completely wrong.
To fix this, we need something else than your patch.
^ permalink raw reply
* Re: [PATCH] stmmac: priv->lock can be used uninitialized
From: Peppe CAVALLARO @ 2010-11-30 14:12 UTC (permalink / raw)
To: Vlad Lungu; +Cc: netdev@vger.kernel.org, davem@davemloft.net
In-Reply-To: <9345448e435110e2bdff65bb4322060a6f1f6b63.1291106902.git.vlad.lungu@windriver.com>
Hi Vlad.
On 11/30/2010 9:52 AM, Vlad Lungu wrote:
>
> To reproduce: if connman (http://connman.net/) is started,
> inserting the stmmac module triggers a "BUG: spinlock bad magic on CPU#0".
>
> Registering the device in stmmac_probe() sends a notification to connman
> which brings the interface up before the lock is initialized.
>
I've never tested connman on our ST platforms.
I've never seen this problems too.
I've no concerns on it so it could be applied.
Regards
Peppe
> Signed-off-by: Vlad Lungu <vlad.lungu@windriver.com>
> ---
> drivers/net/stmmac/stmmac_main.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/stmmac/stmmac_main.c
> b/drivers/net/stmmac/stmmac_main.c
> index 730a6fd..e960501 100644
> --- a/drivers/net/stmmac/stmmac_main.c
> +++ b/drivers/net/stmmac/stmmac_main.c
> @@ -1516,6 +1516,8 @@ static int stmmac_probe(struct net_device *dev)
> pr_warning("\tno valid MAC address;"
> "please, use ifconfig or nwhwconfig!\n");
>
> + spin_lock_init(&priv->lock);
> +
> ret = register_netdev(dev);
> if (ret) {
> pr_err("%s: ERROR %i registering the device\n",
> @@ -1527,8 +1529,6 @@ static int stmmac_probe(struct net_device *dev)
> dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
> (dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
>
> - spin_lock_init(&priv->lock);
> -
> return ret;
> }
>
> --
> 1.6.0.2
>
^ permalink raw reply
* Re: [PATCH 2/2] af_packet: replace struct pgv with char **
From: Eric Dumazet @ 2010-11-30 14:16 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, Jiri Pirko, Neil Horman, netdev
In-Reply-To: <1291125457-14427-1-git-send-email-xiaosuo@gmail.com>
Le mardi 30 novembre 2010 à 21:57 +0800, Changli Gao a écrit :
> As we can check if an address is vmalloc address with is_vmalloc_addr(),
> we can replace struct pgv with char **. Then we may get more pg_vecs.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> net/packet/af_packet.c | 50 ++++++++++++++++---------------------------------
> 1 file changed, 17 insertions(+), 33 deletions(-)
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 0171b20..a26f981 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -164,14 +164,8 @@ struct packet_mreq_max {
> static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
> int closing, int tx_ring);
>
> -#define PGV_FROM_VMALLOC 1
> -struct pgv {
> - char *buffer;
> - unsigned char flags;
> -};
> -
This patch is premature.
Also, keep the struct pgv, even if it has a single field, since having
types is good to read the sources.
We could have 'void *' or 'char *' everywhere, it is not nice...
- pg_vec = kcalloc(block_nr, sizeof(struct pgv), GFP_KERNEL);
+ pg_vec = kcalloc(block_nr, sizeof(char *), GFP_KERNEL);
I prefer to have :
pg_vec = kcalloc(block_nr, sizeof(struct pgv), GFP_KERNEL);
^ permalink raw reply
* PATCH: fix hang in dmfe driver on sending of big packet (linux-2.6.35)
From: Alexander V. Lukyanov @ 2010-11-30 13:38 UTC (permalink / raw)
To: netdev, tori
[-- Attachment #1: Type: text/plain, Size: 170 bytes --]
Hello!
This patch fixes hang in dmfe driver on attempt of sending a big packet.
Without this patch the code stops the queue and never wakes it again.
--
Alexander.
[-- Attachment #2: dmfe.patch --]
[-- Type: text/plain, Size: 602 bytes --]
--- dmfe.c.1 2010-11-30 16:21:52.758465207 +0300
+++ dmfe.c 2010-11-30 16:24:45.301468601 +0300
@@ -687,18 +687,18 @@
unsigned long flags;
DMFE_DBUG(0, "dmfe_start_xmit", 0);
- /* Resource flag check */
- netif_stop_queue(dev);
-
/* Too large packet check */
if (skb->len > MAX_PACKET_SIZE) {
pr_err("big packet = %d\n", (u16)skb->len);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}
+ /* Resource flag check */
+ netif_stop_queue(dev);
+
spin_lock_irqsave(&db->lock, flags);
/* No Tx resource check, it never happen nromally */
if (db->tx_queue_cnt >= TX_FREE_DESC_CNT) {
^ permalink raw reply
* Re: [PATCH 1/2] af_packet: use vmalloc_to_page() instead for the addresss returned by vmalloc()
From: Changli Gao @ 2010-11-30 14:27 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Jiri Pirko, Neil Horman, netdev
In-Reply-To: <1291126341.2904.82.camel@edumazet-laptop>
On Tue, Nov 30, 2010 at 10:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 30 novembre 2010 à 21:56 +0800, Changli Gao a écrit :
>> The following commit causes the pgv->buffer may point to the memory
>> returned by vmalloc(). And we can't use virt_to_page() for the vmalloc
>> address.
>>
>> This patch introduces a new inline function pgv_to_page(), which calls
>> vmalloc_to_page() for the vmalloc address, and virt_to_page() for the
>> __get_free_pages address.
>>
>> commit 0e3125c755445664f00ad036e4fc2cd32fd52877
>> Author: Neil Horman <nhorman@tuxdriver.com>
>> Date: Tue Nov 16 10:26:47 2010 -0800
>>
>> packet: Enhance AF_PACKET implementation to not require high order contiguous memory allocation (v4)
>>
>
> nice catch.
>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>> net/packet/af_packet.c | 21 ++++++++++++++-------
>> 1 file changed, 14 insertions(+), 7 deletions(-)
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 422705d..0171b20 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -224,6 +224,13 @@ struct packet_skb_cb {
>>
>> #define PACKET_SKB_CB(__skb) ((struct packet_skb_cb *)((__skb)->cb))
>>
>> +static inline struct page *pgv_to_page(void *addr)
>> +{
>> + if (is_vmalloc_addr(addr))
>> + return vmalloc_to_page(addr);
>
> Hmm, I am wondering if calling vmalloc_to_page(addr) several times for
> each packet is not too expensive ? I believe it is.
>
> What about caching "struct page *" pointer somewhere ?
>
> Then later we have :
>
>> - p_start = virt_to_page(h.raw);
>> - p_end = virt_to_page(h_end);
>> + p_start = pgv_to_page(h.raw);
>> + p_end = pgv_to_page(h_end);
>> while (p_start <= p_end) {
>> flush_dcache_page(p_start);
>> p_start++;
>
> This was OK before Neil patch... after vmalloc(), assumption that
> p_start can be incremented is completely wrong.
>
> To fix this, we need something else than your patch.
>
Yes, you are right, and tpacket_fill_packet() has the same issue. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH 2/2] af_packet: replace struct pgv with char **
From: Changli Gao @ 2010-11-30 14:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Jiri Pirko, Neil Horman, netdev
In-Reply-To: <1291126568.2904.85.camel@edumazet-laptop>
On Tue, Nov 30, 2010 at 10:16 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 30 novembre 2010 à 21:57 +0800, Changli Gao a écrit :
>> As we can check if an address is vmalloc address with is_vmalloc_addr(),
>> we can replace struct pgv with char **. Then we may get more pg_vecs.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>> net/packet/af_packet.c | 50 ++++++++++++++++---------------------------------
>> 1 file changed, 17 insertions(+), 33 deletions(-)
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 0171b20..a26f981 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -164,14 +164,8 @@ struct packet_mreq_max {
>> static int packet_set_ring(struct sock *sk, struct tpacket_req *req,
>> int closing, int tx_ring);
>>
>> -#define PGV_FROM_VMALLOC 1
>> -struct pgv {
>> - char *buffer;
>> - unsigned char flags;
>> -};
>> -
>
> This patch is premature.
>
> Also, keep the struct pgv, even if it has a single field, since having
> types is good to read the sources.
>
> We could have 'void *' or 'char *' everywhere, it is not nice...
>
>
> - pg_vec = kcalloc(block_nr, sizeof(struct pgv), GFP_KERNEL);
> + pg_vec = kcalloc(block_nr, sizeof(char *), GFP_KERNEL);
>
> I prefer to have :
>
> pg_vec = kcalloc(block_nr, sizeof(struct pgv), GFP_KERNEL);
>
OK. I'll respin it and the previous one in the same serial to address
the issues mentioned by you. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH 1/2] af_packet: use vmalloc_to_page() instead for the addresss returned by vmalloc()
From: Neil Horman @ 2010-11-30 14:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Changli Gao, David S. Miller, Jiri Pirko, netdev
In-Reply-To: <1291126341.2904.82.camel@edumazet-laptop>
On Tue, Nov 30, 2010 at 03:12:21PM +0100, Eric Dumazet wrote:
> Le mardi 30 novembre 2010 à 21:56 +0800, Changli Gao a écrit :
> > The following commit causes the pgv->buffer may point to the memory
> > returned by vmalloc(). And we can't use virt_to_page() for the vmalloc
> > address.
> >
> > This patch introduces a new inline function pgv_to_page(), which calls
> > vmalloc_to_page() for the vmalloc address, and virt_to_page() for the
> > __get_free_pages address.
> >
> > commit 0e3125c755445664f00ad036e4fc2cd32fd52877
> > Author: Neil Horman <nhorman@tuxdriver.com>
> > Date: Tue Nov 16 10:26:47 2010 -0800
> >
> > packet: Enhance AF_PACKET implementation to not require high order contiguous memory allocation (v4)
> >
>
> nice catch.
>
Ouch, yes, thanks.
> > Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> > ---
> > net/packet/af_packet.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 422705d..0171b20 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -224,6 +224,13 @@ struct packet_skb_cb {
> >
> > #define PACKET_SKB_CB(__skb) ((struct packet_skb_cb *)((__skb)->cb))
> >
> > +static inline struct page *pgv_to_page(void *addr)
> > +{
> > + if (is_vmalloc_addr(addr))
> > + return vmalloc_to_page(addr);
>
> Hmm, I am wondering if calling vmalloc_to_page(addr) several times for
> each packet is not too expensive ? I believe it is.
>
> What about caching "struct page *" pointer somewhere ?
>
> Then later we have :
>
> > - p_start = virt_to_page(h.raw);
> > - p_end = virt_to_page(h_end);
> > + p_start = pgv_to_page(h.raw);
> > + p_end = pgv_to_page(h_end);
> > while (p_start <= p_end) {
> > flush_dcache_page(p_start);
> > p_start++;
>
> This was OK before Neil patch... after vmalloc(), assumption that
> p_start can be incremented is completely wrong.
>
> To fix this, we need something else than your patch.
>
Off the top of my head, I would think that pgv_to_page could be prototyped such
that it could accept addr, offset and struct page ** arguments. That way we can
track the current page that we're mapped to, lowering the number of calls to
vmalloc_to_page, and we can still use an increment like we do above (as long as
its wrapped in a subsequent call to pgv_to_page)
Neil
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH] net: Fix drivers advertising HW_CSUM feature to use csum_start
From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw)
To: netdev; +Cc: Ben Hutchings, Jesse Gross
In-Reply-To: <fddashfjkasdhklfjashlkjfs@rechot.qmqm.pl>
Some drivers are using skb_transport_offset(skb) instead of skb->csum_start.
This does not matter now, but if someone implements checksumming of
encapsulated packets then this will break silently.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/atl1c/atl1c_main.c | 2 +-
drivers/net/atl1e/atl1e_main.c | 2 +-
drivers/net/atlx/atl1.c | 2 +-
drivers/net/cassini.c | 2 +-
drivers/net/e1000/e1000_main.c | 2 +-
drivers/net/e1000e/netdev.c | 2 +-
drivers/net/enic/enic_main.c | 2 +-
drivers/net/ixgb/ixgb_main.c | 2 +-
drivers/net/ll_temac_main.c | 2 +-
drivers/net/macvtap.c | 3 +--
drivers/net/myri10ge/myri10ge.c | 2 +-
drivers/net/niu.c | 2 +-
drivers/net/skge.c | 2 +-
drivers/net/sungem.c | 2 +-
drivers/net/sunhme.c | 2 +-
drivers/net/tun.c | 2 +-
drivers/net/usb/smsc95xx.c | 7 +++----
drivers/net/virtio_net.c | 2 +-
drivers/net/vmxnet3/vmxnet3_drv.c | 4 ++--
include/linux/skbuff.h | 5 +++++
20 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
index 09b099b..e48ea95 100644
--- a/drivers/net/atl1c/atl1c_main.c
+++ b/drivers/net/atl1c/atl1c_main.c
@@ -2078,7 +2078,7 @@ static int atl1c_tso_csum(struct atl1c_adapter *adapter,
check_sum:
if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
u8 css, cso;
- cso = skb_transport_offset(skb);
+ cso = skb_checksum_start_offset(skb);
if (unlikely(cso & 0x1)) {
if (netif_msg_tx_err(adapter))
diff --git a/drivers/net/atl1e/atl1e_main.c b/drivers/net/atl1e/atl1e_main.c
index ef6349b..e28f8ba 100644
--- a/drivers/net/atl1e/atl1e_main.c
+++ b/drivers/net/atl1e/atl1e_main.c
@@ -1649,7 +1649,7 @@ check_sum:
if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
u8 css, cso;
- cso = skb_transport_offset(skb);
+ cso = skb_checksum_start_offset(skb);
if (unlikely(cso & 0x1)) {
netdev_err(adapter->netdev,
"payload offset should not ant event number\n");
diff --git a/drivers/net/atlx/atl1.c b/drivers/net/atlx/atl1.c
index 5336310..def8df8 100644
--- a/drivers/net/atlx/atl1.c
+++ b/drivers/net/atlx/atl1.c
@@ -2174,7 +2174,7 @@ static int atl1_tx_csum(struct atl1_adapter *adapter, struct sk_buff *skb,
u8 css, cso;
if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
- css = (u8) (skb->csum_start - skb_headroom(skb));
+ css = skb_checksum_start_offset(skb);
cso = css + (u8) skb->csum_offset;
if (unlikely(css & 0x1)) {
/* L1 hardware requires an even number here */
diff --git a/drivers/net/cassini.c b/drivers/net/cassini.c
index d6b6d6a..35568c1 100644
--- a/drivers/net/cassini.c
+++ b/drivers/net/cassini.c
@@ -2788,7 +2788,7 @@ static inline int cas_xmit_tx_ringN(struct cas *cp, int ring,
ctrl = 0;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- const u64 csum_start_off = skb_transport_offset(skb);
+ const u64 csum_start_off = skb_checksum_start_offset(skb);
const u64 csum_stuff_off = csum_start_off + skb->csum_offset;
ctrl = TX_DESC_CSUM_EN |
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index dcb7f82..9426e04 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2720,7 +2720,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter,
break;
}
- css = skb_transport_offset(skb);
+ css = skb_checksum_start_offset(skb);
i = tx_ring->next_to_use;
buffer_info = &tx_ring->buffer_info[i];
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 0adcb79..9bb9406 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4473,7 +4473,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter, struct sk_buff *skb)
break;
}
- css = skb_transport_offset(skb);
+ css = skb_checksum_start_offset(skb);
i = tx_ring->next_to_use;
buffer_info = &tx_ring->buffer_info[i];
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 9f293fa..78e9d8b 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -702,7 +702,7 @@ static inline void enic_queue_wq_skb_csum_l4(struct enic *enic,
{
unsigned int head_len = skb_headlen(skb);
unsigned int len_left = skb->len - head_len;
- unsigned int hdr_len = skb_transport_offset(skb);
+ unsigned int hdr_len = skb_checksum_start_offset(skb);
unsigned int csum_offset = hdr_len + skb->csum_offset;
int eop = (len_left == 0);
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 211a169..23ec89e 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -1250,7 +1250,7 @@ ixgb_tx_csum(struct ixgb_adapter *adapter, struct sk_buff *skb)
if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
struct ixgb_buffer *buffer_info;
- css = skb_transport_offset(skb);
+ css = skb_checksum_start_offset(skb);
cso = css + skb->csum_offset;
i = adapter->tx_ring.next_to_use;
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index 9f8e702..e2c2a72 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -692,7 +692,7 @@ static int temac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
cur_p->app0 = 0;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- unsigned int csum_start_off = skb_transport_offset(skb);
+ unsigned int csum_start_off = skb_checksum_start_offset(skb);
unsigned int csum_index_off = csum_start_off + skb->csum_offset;
cur_p->app0 |= 1; /* TX Checksum Enabled */
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 4256727..21845af 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -504,8 +504,7 @@ static int macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
if (skb->ip_summed == CHECKSUM_PARTIAL) {
vnet_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
- vnet_hdr->csum_start = skb->csum_start -
- skb_headroom(skb);
+ vnet_hdr->csum_start = skb_checksum_start_offset(skb);
vnet_hdr->csum_offset = skb->csum_offset;
} /* else everything is zero */
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index 8524cc4..c504e29 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -2736,7 +2736,7 @@ again:
odd_flag = 0;
flags = (MXGEFW_FLAGS_NO_TSO | MXGEFW_FLAGS_FIRST);
if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
- cksum_offset = skb_transport_offset(skb);
+ cksum_offset = skb_checksum_start_offset(skb);
pseudo_hdr_offset = cksum_offset + skb->csum_offset;
/* If the headers are excessively large, then we must
* fall back to a software checksum */
diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 781e368..f54d358 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -6589,7 +6589,7 @@ static u64 niu_compute_tx_flags(struct sk_buff *skb, struct ethhdr *ehdr,
(ip_proto == IPPROTO_UDP ?
TXHDR_CSUM_UDP : TXHDR_CSUM_SCTP));
- start = skb_transport_offset(skb) -
+ start = skb_checksum_start_offset(skb) -
(pad_bytes + sizeof(struct tx_pkt_hdr));
stuff = start + skb->csum_offset;
diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 220e039..1959438 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -2764,7 +2764,7 @@ static netdev_tx_t skge_xmit_frame(struct sk_buff *skb,
td->dma_hi = map >> 32;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- const int offset = skb_transport_offset(skb);
+ const int offset = skb_checksum_start_offset(skb);
/* This seems backwards, but it is what the sk98lin
* does. Looks like hardware is wrong?
diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 4ceb3cf..4842fca 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -1004,7 +1004,7 @@ static netdev_tx_t gem_start_xmit(struct sk_buff *skb,
ctrl = 0;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- const u64 csum_start_off = skb_transport_offset(skb);
+ const u64 csum_start_off = skb_checksum_start_offset(skb);
const u64 csum_stuff_off = csum_start_off + skb->csum_offset;
ctrl = (TXDCTRL_CENAB |
diff --git a/drivers/net/sunhme.c b/drivers/net/sunhme.c
index 5e28c41..55bbb9c 100644
--- a/drivers/net/sunhme.c
+++ b/drivers/net/sunhme.c
@@ -2266,7 +2266,7 @@ static netdev_tx_t happy_meal_start_xmit(struct sk_buff *skb,
tx_flags = TXFLAG_OWN;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- const u32 csum_start_off = skb_transport_offset(skb);
+ const u32 csum_start_off = skb_checksum_start_offset(skb);
const u32 csum_stuff_off = csum_start_off + skb->csum_offset;
tx_flags = (TXFLAG_OWN | TXFLAG_CSENABLE |
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 55f3a3e..7599c45 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -757,7 +757,7 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
if (skb->ip_summed == CHECKSUM_PARTIAL) {
gso.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
- gso.csum_start = skb->csum_start - skb_headroom(skb);
+ gso.csum_start = skb_checksum_start_offset(skb);
gso.csum_offset = skb->csum_offset;
} /* else everything is zero */
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 65cb1ab..bc86f4b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1163,9 +1163,8 @@ static int smsc95xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
static u32 smsc95xx_calc_csum_preamble(struct sk_buff *skb)
{
- int len = skb->data - skb->head;
- u16 high_16 = (u16)(skb->csum_offset + skb->csum_start - len);
- u16 low_16 = (u16)(skb->csum_start - len);
+ u16 low_16 = (u16)skb_checksum_start_offset(skb);
+ u16 high_16 = low_16 + skb->csum_offset;
return (high_16 << 16) | low_16;
}
@@ -1193,7 +1192,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
if (skb->len <= 45) {
/* workaround - hardware tx checksum does not work
* properly with extremely small packets */
- long csstart = skb->csum_start - skb_headroom(skb);
+ long csstart = skb_checksum_start_offset(skb);
__wsum calc = csum_partial(skb->data + csstart,
skb->len - csstart, 0);
*((__sum16 *)(skb->data + csstart
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b6d4028..90a23e4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -519,7 +519,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
if (skb->ip_summed == CHECKSUM_PARTIAL) {
hdr->hdr.flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
- hdr->hdr.csum_start = skb->csum_start - skb_headroom(skb);
+ hdr->hdr.csum_start = skb_checksum_start_offset(skb);
hdr->hdr.csum_offset = skb->csum_offset;
} else {
hdr->hdr.flags = 0;
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 65860a9..3b61e21 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -798,7 +798,7 @@ vmxnet3_parse_and_copy_hdr(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
{
struct Vmxnet3_TxDataDesc *tdd;
- if (ctx->mss) {
+ if (ctx->mss) { /* GSO */
ctx->eth_ip_hdr_size = skb_transport_offset(skb);
ctx->l4_hdr_size = ((struct tcphdr *)
skb_transport_header(skb))->doff * 4;
@@ -807,7 +807,7 @@ vmxnet3_parse_and_copy_hdr(struct sk_buff *skb, struct vmxnet3_tx_queue *tq,
unsigned int pull_size;
if (skb->ip_summed == CHECKSUM_PARTIAL) {
- ctx->eth_ip_hdr_size = skb_transport_offset(skb);
+ ctx->eth_ip_hdr_size = skb_checksum_start_offset(skb);
if (ctx->ipv4) {
struct iphdr *iph = (struct iphdr *)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 19f37a6..0491da5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1355,6 +1355,11 @@ static inline void skb_set_mac_header(struct sk_buff *skb, const int offset)
}
#endif /* NET_SKBUFF_DATA_USES_OFFSET */
+static inline int skb_checksum_start_offset(const struct sk_buff *skb)
+{
+ return skb->csum_start - skb_headroom(skb);
+}
+
static inline int skb_transport_offset(const struct sk_buff *skb)
{
return skb_transport_header(skb) - skb->data;
^ permalink raw reply related
* [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features
From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw)
To: netdev; +Cc: Ben Hutchings, Jesse Gross
In-Reply-To: <fddashfjkasdhklfjashlkjfs@rechot.qmqm.pl>
NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
some drivers miss the difference. Fix this and also fix UFO dependency
on checksumming offload as it makes the same mistake in assumptions.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/benet/be_main.c | 6 ++++--
drivers/net/bnx2x/bnx2x_main.c | 4 ++--
drivers/net/jme.c | 16 +++++++++-------
drivers/net/pch_gbe/pch_gbe_ethtool.c | 19 +------------------
drivers/net/pch_gbe/pch_gbe_main.c | 6 +++---
drivers/net/sc92031.c | 3 ++-
drivers/net/stmmac/stmmac_ethtool.c | 12 +-----------
drivers/net/stmmac/stmmac_main.c | 5 +++--
drivers/net/vxge/vxge-ethtool.c | 2 +-
drivers/net/vxge/vxge-main.c | 2 +-
net/core/dev.c | 7 +++++--
net/core/ethtool.c | 4 +++-
12 files changed, 35 insertions(+), 51 deletions(-)
diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 102567e..2754280 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -2583,10 +2583,12 @@ static void be_netdev_init(struct net_device *netdev)
int i;
netdev->features |= NETIF_F_SG | NETIF_F_HW_VLAN_RX | NETIF_F_TSO |
- NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER | NETIF_F_HW_CSUM |
+ NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
+ NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
NETIF_F_GRO | NETIF_F_TSO6;
- netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM;
+ netdev->vlan_features |= NETIF_F_SG | NETIF_F_TSO |
+ NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
if (lancer_chip(adapter))
netdev->vlan_features |= NETIF_F_TSO6;
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index f53edfd..40ce95a 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
dev->netdev_ops = &bnx2x_netdev_ops;
bnx2x_set_ethtool_ops(dev);
dev->features |= NETIF_F_SG;
- dev->features |= NETIF_F_HW_CSUM;
+ dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
if (bp->flags & USING_DAC_FLAG)
dev->features |= NETIF_F_HIGHDMA;
dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
@@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
dev->vlan_features |= NETIF_F_SG;
- dev->vlan_features |= NETIF_F_HW_CSUM;
+ dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
if (bp->flags & USING_DAC_FLAG)
dev->vlan_features |= NETIF_F_HIGHDMA;
dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index c57d9a4..ad0935c 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
}
if (new_mtu > 1900) {
- netdev->features &= ~(NETIF_F_HW_CSUM |
- NETIF_F_TSO |
- NETIF_F_TSO6);
+ netdev->features &= ~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM |
+ NETIF_F_TSO | NETIF_F_TSO6);
} else {
if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
- netdev->features |= NETIF_F_HW_CSUM;
+ netdev->features |= NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
if (test_bit(JME_FLAG_TSO, &jme->flags))
netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
}
@@ -2514,10 +2513,12 @@ jme_set_tx_csum(struct net_device *netdev, u32 on)
if (on) {
set_bit(JME_FLAG_TXCSUM, &jme->flags);
if (netdev->mtu <= 1900)
- netdev->features |= NETIF_F_HW_CSUM;
+ netdev->features |=
+ NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
} else {
clear_bit(JME_FLAG_TXCSUM, &jme->flags);
- netdev->features &= ~NETIF_F_HW_CSUM;
+ netdev->features &=
+ ~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM);
}
return 0;
@@ -2797,7 +2798,8 @@ jme_init_one(struct pci_dev *pdev,
netdev->netdev_ops = &jme_netdev_ops;
netdev->ethtool_ops = &jme_ethtool_ops;
netdev->watchdog_timeo = TX_TIMEOUT;
- netdev->features = NETIF_F_HW_CSUM |
+ netdev->features = NETIF_F_IP_CSUM |
+ NETIF_F_IPV6_CSUM |
NETIF_F_SG |
NETIF_F_TSO |
NETIF_F_TSO6 |
diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
index c8cc32c..c8c873b 100644
--- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
+++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
@@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
}
/**
- * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
- * @netdev: Network interface device structure
- * Returns
- * true(1): Checksum On
- * false(0): Checksum Off
- */
-static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
-{
- return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-/**
* pch_gbe_set_tx_csum - Turn transmit checksums on or off
* @netdev: Network interface device structure
* @data: Checksum on[true] or off[false]
@@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
struct pch_gbe_adapter *adapter = netdev_priv(netdev);
adapter->tx_csum = data;
- if (data)
- netdev->features |= NETIF_F_HW_CSUM;
- else
- netdev->features &= ~NETIF_F_HW_CSUM;
- return 0;
+ return ethtool_op_set_tx_ipv6_csum(netdev, data);
}
/**
@@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
.set_pauseparam = pch_gbe_set_pauseparam,
.get_rx_csum = pch_gbe_get_rx_csum,
.set_rx_csum = pch_gbe_set_rx_csum,
- .get_tx_csum = pch_gbe_get_tx_csum,
.set_tx_csum = pch_gbe_set_tx_csum,
.get_strings = pch_gbe_get_strings,
.get_ethtool_stats = pch_gbe_get_ethtool_stats,
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index afb7506..58e7903 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -2319,7 +2319,7 @@ static int pch_gbe_probe(struct pci_dev *pdev,
netdev->watchdog_timeo = PCH_GBE_WATCHDOG_PERIOD;
netif_napi_add(netdev, &adapter->napi,
pch_gbe_napi_poll, PCH_GBE_RX_WEIGHT);
- netdev->features = NETIF_F_HW_CSUM | NETIF_F_GRO;
+ netdev->features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_GRO;
pch_gbe_set_ethtool_ops(netdev);
pch_gbe_mac_reset_hw(&adapter->hw);
@@ -2358,9 +2358,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
pch_gbe_check_options(adapter);
if (adapter->tx_csum)
- netdev->features |= NETIF_F_HW_CSUM;
+ netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
else
- netdev->features &= ~NETIF_F_HW_CSUM;
+ netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
/* initialize the wol settings based on the eeprom settings */
adapter->wake_up_evt = PCH_GBE_WL_INIT_SETTING;
diff --git a/drivers/net/sc92031.c b/drivers/net/sc92031.c
index 417adf3..76290a8 100644
--- a/drivers/net/sc92031.c
+++ b/drivers/net/sc92031.c
@@ -1449,7 +1449,8 @@ static int __devinit sc92031_probe(struct pci_dev *pdev,
dev->irq = pdev->irq;
/* faked with skb_copy_and_csum_dev */
- dev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA;
+ dev->features = NETIF_F_SG | NETIF_F_HIGHDMA |
+ NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
dev->netdev_ops = &sc92031_netdev_ops;
dev->watchdog_timeo = TX_TIMEOUT;
diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
index f2695fd..fd719ed 100644
--- a/drivers/net/stmmac/stmmac_ethtool.c
+++ b/drivers/net/stmmac/stmmac_ethtool.c
@@ -197,16 +197,6 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
}
}
-static int stmmac_ethtool_set_tx_csum(struct net_device *netdev, u32 data)
-{
- if (data)
- netdev->features |= NETIF_F_HW_CSUM;
- else
- netdev->features &= ~NETIF_F_HW_CSUM;
-
- return 0;
-}
-
static u32 stmmac_ethtool_get_rx_csum(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
@@ -370,7 +360,7 @@ static struct ethtool_ops stmmac_ethtool_ops = {
.get_link = ethtool_op_get_link,
.get_rx_csum = stmmac_ethtool_get_rx_csum,
.get_tx_csum = ethtool_op_get_tx_csum,
- .set_tx_csum = stmmac_ethtool_set_tx_csum,
+ .set_tx_csum = ethtool_op_set_tx_ipv6_csum,
.get_sg = ethtool_op_get_sg,
.set_sg = ethtool_op_set_sg,
.get_pauseparam = stmmac_get_pauseparam,
diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
index 730a6fd..bfc2d12 100644
--- a/drivers/net/stmmac/stmmac_main.c
+++ b/drivers/net/stmmac/stmmac_main.c
@@ -1494,7 +1494,8 @@ static int stmmac_probe(struct net_device *dev)
dev->netdev_ops = &stmmac_netdev_ops;
stmmac_set_ethtool_ops(dev);
- dev->features |= (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA);
+ dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA |
+ NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
dev->watchdog_timeo = msecs_to_jiffies(watchdog);
#ifdef STMMAC_VLAN_TAG_USED
/* Both mac100 and gmac support receive VLAN tag detection */
@@ -1525,7 +1526,7 @@ static int stmmac_probe(struct net_device *dev)
DBG(probe, DEBUG, "%s: Scatter/Gather: %s - HW checksums: %s\n",
dev->name, (dev->features & NETIF_F_SG) ? "on" : "off",
- (dev->features & NETIF_F_HW_CSUM) ? "on" : "off");
+ (dev->features & NETIF_F_IP_CSUM) ? "on" : "off");
spin_lock_init(&priv->lock);
diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
index bc9bd10..5ece50d 100644
--- a/drivers/net/vxge/vxge-ethtool.c
+++ b/drivers/net/vxge/vxge-ethtool.c
@@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
.get_rx_csum = vxge_get_rx_csum,
.set_rx_csum = vxge_set_rx_csum,
.get_tx_csum = ethtool_op_get_tx_csum,
- .set_tx_csum = ethtool_op_set_tx_hw_csum,
+ .set_tx_csum = ethtool_op_set_tx_csum,
.get_sg = ethtool_op_get_sg,
.set_sg = ethtool_op_set_sg,
.get_tso = ethtool_op_get_tso,
diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
index a21dae1..9f6d379 100644
--- a/drivers/net/vxge/vxge-main.c
+++ b/drivers/net/vxge/vxge-main.c
@@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
ndev->features |= NETIF_F_SG;
- ndev->features |= NETIF_F_HW_CSUM;
+ ndev->features |= NETIF_F_IP_CSUM;
vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
"%s : checksuming enabled", __func__);
diff --git a/net/core/dev.c b/net/core/dev.c
index 3259d2c..622f85a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
}
if (features & NETIF_F_UFO) {
- if (!(features & NETIF_F_GEN_CSUM)) {
+ /* maybe split UFO into V4 and V6? */
+ if (!((features & NETIF_F_GEN_CSUM) ||
+ (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+ == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
if (name)
printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
- "since no NETIF_F_HW_CSUM feature.\n",
+ "since no checksum offload features.\n",
name);
features &= ~NETIF_F_UFO;
}
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 956a9f4..d5bc2881 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
return -EFAULT;
if (edata.data && !(dev->features & NETIF_F_SG))
return -EINVAL;
- if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
+ if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
+ (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
+ == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
return -EINVAL;
return dev->ethtool_ops->set_ufo(dev, edata.data);
}
^ permalink raw reply related
* Re: Broken TX checksumming offloads
From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw)
To: netdev; +Cc: Ben Hutchings, Jesse Gross
In-Reply-To: <AANLkTi=U51Y7rsQXr+_6TreUks_MyRMdRU=TtdxKOG2Z@mail.gmail.com>
On Mon, Nov 29, 2010 at 06:35:44PM -0800, Jesse Gross wrote:
> 2010/11/29 Ben Hutchings <bhutchings@solarflare.com>:
> > On Mon, 2010-11-29 at 19:17 +0100, Michał Mirosław wrote:
> >> Hi!
> >>
> >> Unless I'm horribly mistaken, generic HW checksumming works as follows:
> >> - driver sets netdev->features & NETIF_F_HW_CSUM to indicate support
> >> for generic checksumming; if the flag is not set, networking core
> >> will checksum skb before calling ndo_start_xmit (let's ignore
> >> other checksumming options for now) and not pass skb with
> >> skb->ip_summed == CHECKSUM_PARTIAL
> >> - ndo_start_xmit() should use skb->csum_start and skb->csum_offset
> >> (or skb->csum) to update checksum in software or instruct HW to do so
> >>
> >> Looking at pch_gbe_xmit_frame() and its callee - pch_gbe_tx_queue() it
> >> looks like the driver should set NETIF_F_IP_CSUM instead of NETIF_F_HW_CSUM
> >> feature.
> >>
> >> Similar thing happens in ixgbe driver: it sets NETIF_F_HW_CSUM and checks
> >> for skb->ip_summed == CHECKSUM_PARTIAL, but then just warns on protocols
> >> other that TCP and SCTP (see: ixgbe_psum()).
> >
> > AFAIK only {TCP,UDP}/IPv{4,6} use the simple 16-bit checksum algorithm
> > that NETIF_F_HW_CSUM implies, so in practice it is equivalent to
> > NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM even though it doesn't mean the same
> > thing.
>
> There are a few cases that I can think of where the extra generality
> of NETIF_F_HW_CSUM is useful:
>
> * The checksum in GRE (maybe other protocols as well) uses the same
> algorithm as TCP/UDP. In theory this means that we could use hardware
> offloading for GRE on cards that have NETIF_F_HW_CSUM. The one issue
> is that NETIF_F_IP_CSUM really means TCP/UDP checksumming but we only
> test if the protocol is IP. This means that other IP based protocols
> (like GRE) can't use checksum offloading since there is no software
> fallback path.
>
> * TCP/UDP packets that are deeply encapsulated. Currently we can
> express that checksum offloading is supported in one level of vlan
> encapsulation through vlan_features. However, a NIC that exposes
> NETIF_F_HW_CSUM should in theory be able to checksum over a packet
> that has an arbitrary number of vlan tags in front of it, a GRE
> header, or any other type of encapsulation.
So the following patches should fix what's in tree regarding
NETIF_F_HW_CSUM interpretation. The patches have no interdependencies,
so can be applied independently. The last one is a cleanup that tagged
along.
Drivers that didn't care about csum_{start,offset}:
benet
bnx2x
jme
pch_gbe
sc92031
stmmac
vxge
Best Regards,
Michał Mirosław
^ permalink raw reply
* [PATCH] net: Move check of checksum features to netdev_fix_features()
From: MichałMirosław @ 2010-11-30 14:23 UTC (permalink / raw)
To: netdev; +Cc: Ben Hutchings, Jesse Gross
In-Reply-To: <fddashfjkasdhklfjashlkjfs@rechot.qmqm.pl>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
diff --git a/net/core/dev.c b/net/core/dev.c
index 622f85a..c64a848 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5023,6 +5023,23 @@ static void rollback_registered(struct net_device *dev)
unsigned long netdev_fix_features(unsigned long features, const char *name)
{
+ /* Fix illegal checksum combinations */
+ if ((features & NETIF_F_HW_CSUM) &&
+ (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+ if (name)
+ printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
+ name);
+ features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+ }
+
+ if ((features & NETIF_F_NO_CSUM) &&
+ (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+ if (name)
+ printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
+ name);
+ features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
+ }
+
/* Fix illegal SG+CSUM combinations. */
if ((features & NETIF_F_SG) &&
!(features & NETIF_F_ALL_CSUM)) {
@@ -5206,21 +5223,6 @@ int register_netdevice(struct net_device *dev)
if (dev->iflink == -1)
dev->iflink = dev->ifindex;
- /* Fix illegal checksum combinations */
- if ((dev->features & NETIF_F_HW_CSUM) &&
- (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
- printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
- dev->name);
- dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
- }
-
- if ((dev->features & NETIF_F_NO_CSUM) &&
- (dev->features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
- printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
- dev->name);
- dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
- }
-
dev->features = netdev_fix_features(dev->features, dev->name);
/* Enable software GSO if SG is supported. */
^ permalink raw reply related
* Re: [PATCH] Net-ethtool : Allow ethtool to set interface in loopback mode.
From: Ben Hutchings @ 2010-11-30 15:01 UTC (permalink / raw)
To: Mahesh Bandewar; +Cc: David Miller, linux-netdev, laurent chavey
In-Reply-To: <AANLkTikVaVv4fkLom+NSQgOUzCd2i3mbHkBNQ6PD4d9Y@mail.gmail.com>
On Tue, 2010-11-30 at 00:00 -0800, Mahesh Bandewar wrote:
> This patch enables ethtool to set the loopback mode on a given
> interface. This is the reworked version of earlier submit (which I
> don't have reference to). By configuring the interface in loopback
> mode in conjunction with a policy route / rule, a userland application
> can stress the egress / ingress path exposing the flows of the change
> in progress and potentially help developer(s) understand the impact of
> those changes without even sending a packet out on the network.
Is the aim to stress the generic egress/ingress code or also to cover
the specific driver in use?
I note that your colleague Laurent Chavey posted a very similar patch
back in April <http://article.gmane.org/gmane.linux.network/157489> but
he emphasised hardware diagnosis.
> Following set of commands illustrates one such example -
> a) ifconfig eth1 192.168.1.1
> b) ip -4 rule add from all iif eth1 lookup 250
> c) ip -4 route add local 0/0 dev lo proto kernel scope host table 250
> d) arp -Ds 192.168.1.100 eth1
> e) arp -Ds 192.168.1.200 eth1
> f) sysctl -w net.ipv4.ip_nonlocal_bind=1
> g) sysctl -w net.ipv4.conf.all.accept_local=1
> # Assuming that the machine has 8 cores
> h) taskset 000f netserver -L 192.168.1.200
> i) taskset 00f0 netperf -t TCP_CRR -L 192.168.1.100 -H 192.168.1.200 -l 30
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> include/linux/ethtool.h | 4 ++++
> net/core/ethtool.c | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 6628a50..7523d45 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -678,6 +678,8 @@ struct ethtool_ops {
> struct ethtool_rxfh_indir *);
> int (*set_rxfh_indir)(struct net_device *,
> const struct ethtool_rxfh_indir *);
> + int (*get_loopback)(struct net_device *, u32 *);
> + int (*set_loopback)(struct net_device *, u32);
> };
> #endif /* __KERNEL__ */
>
> @@ -741,6 +743,8 @@ struct ethtool_ops {
> #define ETHTOOL_GSSET_INFO 0x00000037 /* Get string set info */
> #define ETHTOOL_GRXFHINDIR 0x00000038 /* Get RX flow hash indir'n table */
> #define ETHTOOL_SRXFHINDIR 0x00000039 /* Set RX flow hash indir'n table */
> +#define ETHTOOL_SLOOPBACK 0x0000003a /* Enable / Disable Loopback */
[...]
Where should loopback be done, when enabled? As near as possible to the
host, so it only covers the DMA engines, or as far away as possible, so
it covers most of the MAC/PHY hardware?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features
From: MichałMirosław @ 2010-11-30 15:28 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, Jesse Gross
In-Reply-To: <1291130005.21077.18.camel@bwh-desktop>
On Tue, Nov 30, 2010 at 03:13:25PM +0000, Ben Hutchings wrote:
> On Tue, 2010-11-30 at 15:23 +0100, MichałMirosław wrote:
> > NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > some drivers miss the difference. Fix this and also fix UFO dependency
> > on checksumming offload as it makes the same mistake in assumptions.
> [...]
> > diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
> > index f53edfd..40ce95a 100644
> > --- a/drivers/net/bnx2x/bnx2x_main.c
> > +++ b/drivers/net/bnx2x/bnx2x_main.c
> > @@ -8761,7 +8761,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> > dev->netdev_ops = &bnx2x_netdev_ops;
> > bnx2x_set_ethtool_ops(dev);
> > dev->features |= NETIF_F_SG;
> > - dev->features |= NETIF_F_HW_CSUM;
> > + dev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > if (bp->flags & USING_DAC_FLAG)
> > dev->features |= NETIF_F_HIGHDMA;
> > dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > @@ -8769,7 +8769,7 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
> > dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
> >
> > dev->vlan_features |= NETIF_F_SG;
> > - dev->vlan_features |= NETIF_F_HW_CSUM;
> > + dev->vlan_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> > if (bp->flags & USING_DAC_FLAG)
> > dev->vlan_features |= NETIF_F_HIGHDMA;
> > dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> > diff --git a/drivers/net/jme.c b/drivers/net/jme.c
> > index c57d9a4..ad0935c 100644
> > --- a/drivers/net/jme.c
> > +++ b/drivers/net/jme.c
> > @@ -2076,12 +2076,11 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
> > }
> >
> > if (new_mtu > 1900) {
> > - netdev->features &= ~(NETIF_F_HW_CSUM |
> > - NETIF_F_TSO |
> > - NETIF_F_TSO6);
> > + netdev->features &= ~(NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM |
> > + NETIF_F_TSO | NETIF_F_TSO6);
> > } else {
> > if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
> > - netdev->features |= NETIF_F_HW_CSUM;
> > + netdev->features |= NETIF_F_IP_CSUM | NETIFI_F_IPV6_CSUM;
>
> In this file you've written 'NETIFI_F_IPV6_CSUM' several times. Do try
> compiling your work. :-)
Sorry for that. I compile tested whole series, where another patch I'm working on
just removes this part.
> [...]
> > diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > index c8cc32c..c8c873b 100644
> > --- a/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > +++ b/drivers/net/pch_gbe/pch_gbe_ethtool.c
> > @@ -469,18 +469,6 @@ static int pch_gbe_set_rx_csum(struct net_device *netdev, u32 data)
> > }
> >
> > /**
> > - * pch_gbe_get_tx_csum - Report whether transmit checksums are turned on or off
> > - * @netdev: Network interface device structure
> > - * Returns
> > - * true(1): Checksum On
> > - * false(0): Checksum Off
> > - */
> > -static u32 pch_gbe_get_tx_csum(struct net_device *netdev)
> > -{
> > - return (netdev->features & NETIF_F_HW_CSUM) != 0;
> > -}
> > -
> > -/**
> > * pch_gbe_set_tx_csum - Turn transmit checksums on or off
> > * @netdev: Network interface device structure
> > * @data: Checksum on[true] or off[false]
> > @@ -493,11 +481,7 @@ static int pch_gbe_set_tx_csum(struct net_device *netdev, u32 data)
> > struct pch_gbe_adapter *adapter = netdev_priv(netdev);
> >
> > adapter->tx_csum = data;
> > - if (data)
> > - netdev->features |= NETIF_F_HW_CSUM;
> > - else
> > - netdev->features &= ~NETIF_F_HW_CSUM;
> > - return 0;
> > + return ethtool_op_set_tx_ipv6_csum(netdev, data);
> > }
> >
> > /**
> > @@ -572,7 +556,6 @@ static const struct ethtool_ops pch_gbe_ethtool_ops = {
> > .set_pauseparam = pch_gbe_set_pauseparam,
> > .get_rx_csum = pch_gbe_get_rx_csum,
> > .set_rx_csum = pch_gbe_set_rx_csum,
> > - .get_tx_csum = pch_gbe_get_tx_csum,
> > .set_tx_csum = pch_gbe_set_tx_csum,
> > .get_strings = pch_gbe_get_strings,
> > .get_ethtool_stats = pch_gbe_get_ethtool_stats,
>
> pch_gbe_get_tx_csum can simply be replaced with
> ethtool_op_set_tx_ipv6_csum.
pch_gbe_set_tx_csum() also changes adapter->tx_csum, which I didn't want
to touch in this patch. (I'm assuming you mean ...set_tx_csum not ...get_tx_csum).
> [...]
> > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > index bc9bd10..5ece50d 100644
> > --- a/drivers/net/vxge/vxge-ethtool.c
> > +++ b/drivers/net/vxge/vxge-ethtool.c
> > @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
> > .get_rx_csum = vxge_get_rx_csum,
> > .set_rx_csum = vxge_set_rx_csum,
> > .get_tx_csum = ethtool_op_get_tx_csum,
> > - .set_tx_csum = ethtool_op_set_tx_hw_csum,
> > + .set_tx_csum = ethtool_op_set_tx_csum,
> > .get_sg = ethtool_op_get_sg,
> > .set_sg = ethtool_op_set_sg,
> > .get_tso = ethtool_op_get_tso,
> > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > index a21dae1..9f6d379 100644
> > --- a/drivers/net/vxge/vxge-main.c
> > +++ b/drivers/net/vxge/vxge-main.c
> > @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
> >
> > ndev->features |= NETIF_F_SG;
> >
> > - ndev->features |= NETIF_F_HW_CSUM;
> > + ndev->features |= NETIF_F_IP_CSUM;
> > vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
> > "%s : checksuming enabled", __func__);
> >
> Why are you disabling IPv6 checksum offload? From a quick look at the
> driver, I think the hardware does support it.
In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
is the following code, that suggested otherwise:
if (skb->ip_summed == CHECKSUM_PARTIAL)
vxge_hw_fifo_txdl_cksum_set_bits(dtr,
VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 3259d2c..622f85a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5041,10 +5041,13 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> > }
> >
> > if (features & NETIF_F_UFO) {
> > - if (!(features & NETIF_F_GEN_CSUM)) {
> > + /* maybe split UFO into V4 and V6? */
> > + if (!((features & NETIF_F_GEN_CSUM) ||
> > + (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > + == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> > if (name)
> > printk(KERN_ERR "%s: Dropping NETIF_F_UFO "
> > - "since no NETIF_F_HW_CSUM feature.\n",
> > + "since no checksum offload features.\n",
> > name);
> > features &= ~NETIF_F_UFO;
> > }
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 956a9f4..d5bc2881 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1171,7 +1171,9 @@ static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
> > return -EFAULT;
> > if (edata.data && !(dev->features & NETIF_F_SG))
> > return -EINVAL;
> > - if (edata.data && !(dev->features & NETIF_F_HW_CSUM))
> > + if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
> > + (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
> > + == (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
> > return -EINVAL;
> > return dev->ethtool_ops->set_ufo(dev, edata.data);
> > }
> I believe UFO is for IPv4 only; IPv6 has an entirely different kind of
> fragmentation. So I think the check should be dev->features &
> NETIF_F_V4_CSUM.
net/ipv6/ip6_output.c references NETIF_F_UFO without checking
IPv6 checksumming features.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH] net: Fix too optimistic NETIF_F_HW_CSUM features
From: Michał Mirosław @ 2010-11-30 15:41 UTC (permalink / raw)
To: Ramkrishna Vepa, Sivakumar Subramani, Sreenivasa Honnur,
Jon Mason
Cc: netdev, Ben Hutchings, Jesse Gross
In-Reply-To: <20101130152838.GA26281@rere.qmqm.pl>
On Tue, Nov 30, 2010 at 04:28:38PM +0100, MichałMirosław wrote:
> On Tue, Nov 30, 2010 at 03:13:25PM +0000, Ben Hutchings wrote:
> > On Tue, 2010-11-30 at 15:23 +0100, MichałMirosław wrote:
> > > NETIF_F_HW_CSUM is superset of NETIF_F_IP_CSUM+NETIF_F_IPV6_CSUM, but
> > > some drivers miss the difference. Fix this and also fix UFO dependency
> > > on checksumming offload as it makes the same mistake in assumptions.
[...]
> > > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > > index bc9bd10..5ece50d 100644
> > > --- a/drivers/net/vxge/vxge-ethtool.c
> > > +++ b/drivers/net/vxge/vxge-ethtool.c
> > > @@ -1177,7 +1177,7 @@ static const struct ethtool_ops vxge_ethtool_ops = {
> > > .get_rx_csum = vxge_get_rx_csum,
> > > .set_rx_csum = vxge_set_rx_csum,
> > > .get_tx_csum = ethtool_op_get_tx_csum,
> > > - .set_tx_csum = ethtool_op_set_tx_hw_csum,
> > > + .set_tx_csum = ethtool_op_set_tx_csum,
> > > .get_sg = ethtool_op_get_sg,
> > > .set_sg = ethtool_op_set_sg,
> > > .get_tso = ethtool_op_get_tso,
> > > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > > index a21dae1..9f6d379 100644
> > > --- a/drivers/net/vxge/vxge-main.c
> > > +++ b/drivers/net/vxge/vxge-main.c
> > > @@ -3368,7 +3368,7 @@ static int __devinit vxge_device_register(struct __vxge_hw_device *hldev,
> > >
> > > ndev->features |= NETIF_F_SG;
> > >
> > > - ndev->features |= NETIF_F_HW_CSUM;
> > > + ndev->features |= NETIF_F_IP_CSUM;
> > > vxge_debug_init(vxge_hw_device_trace_level_get(hldev),
> > > "%s : checksuming enabled", __func__);
> > >
> > Why are you disabling IPv6 checksum offload? From a quick look at the
> > driver, I think the hardware does support it.
>
> In vxge_xmit() (at drivers/net/vxge/vxge-main.c:922 in net-next) there
> is the following code, that suggested otherwise:
>
> if (skb->ip_summed == CHECKSUM_PARTIAL)
> vxge_hw_fifo_txdl_cksum_set_bits(dtr,
> VXGE_HW_FIFO_TXD_TX_CKO_IPV4_EN |
> VXGE_HW_FIFO_TXD_TX_CKO_TCP_EN |
> VXGE_HW_FIFO_TXD_TX_CKO_UDP_EN);
>
Lets ask vxge driver maintainters on this.
Does vxge support IPv6 TCP/UDP checksumming offload?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Bonding, GRO and tcp_reordering
From: Ben Hutchings @ 2010-11-30 15:42 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev
In-Reply-To: <20101130135549.GA22688@verge.net.au>
On Tue, 2010-11-30 at 22:55 +0900, Simon Horman wrote:
> Hi,
>
> I just wanted to share what is a rather pleasing,
> though to me somewhat surprising result.
>
> I am testing bonding using balance-rr mode with three physical links to try
> to get > gigabit speed for a single stream. Why? Because I'd like to run
> various tests at > gigabit speed and I don't have any 10G hardware at my
> disposal.
>
> The result I have is that with a 1500 byte MTU, tcp_reordering=3 and both
> LSO and GSO disabled on both the sender and receiver I see:
>
> # netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 1472
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216
> (172.17.60.216) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
>
> 87380 16384 1472 10.01 1646.13 40.01 -1.00 3.982 -1.000
>
> But with GRO enabled on the receiver I see.
>
> # netperf -c -4 -t TCP_STREAM -H 172.17.60.216 -- -m 1472
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216
> (172.17.60.216) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % U us/KB us/KB
>
> 87380 16384 1472 10.01 2613.83 19.32 -1.00 1.211 -1.000
>
> Which is much better than any result I get tweaking tcp_reordering when
> GRO is disabled on the receiver.
Did you also enable TSO/GSO on the sender?
What TSO/GSO will do is to change the round-robin scheduling from one
packet per interface to one super-packet per interface. GRO then
coalesces the physical packets back into a super-packet. The intervals
between receiving super-packets then tend to exceed the difference in
delay between interfaces, hiding the reordering.
If you only enabled GRO then I don't understand this.
> Tweaking tcp_reordering when GRO is enabled on the receiver seems to have
> negligible effect. Which is interesting, because my brief reading on the
> subject indicated that tcp_reordering was the key tuning parameter for
> bonding with balance-rr.
>
> The only other parameter that seemed to have significant effect was to
> increase the mtu. In the case of MTU=9000, GRO seemed to have a negative
> impact on throughput, though a significant positive effect on CPU
> utilisation.
[...]
Increasing MTU also increases the interval between packets on a TCP flow
using maximum segment size so that it is more likely to exceed the
difference in delay.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH 0/5] xfrm: ESP Traffic Flow Confidentiality padding
From: Martin Willi @ 2010-11-30 15:49 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto, netdev
The following patchset adds Traffic Flow Confidentiality padding. The
first patch introduces a new Netlink XFRM attribute to configure TFC via
userspace. The second patch removes an existing padlen option in ESP; It
is not used at all, and I currently don't see the purpose of the field,
nor how it should interact with TFC padding enabled. Patch three and four
implement the padding logic in IPv4 and IPv6 ESP.
Padding is specified with a length to pad the encapsulated data to.
Support for TFC padding as specified in RFC4303 must be negotiated
explicitly by the key management protocol, hence the optional flag. The
fallback with ESP padding field expansion is limited to 255 padding
bytes. If this is insufficient, padding length is randomized to hide
the real length as good as possible.
The last patch adds an option to pad all packets to the PMTU. It works
fine for simple scenarios, but I'm not sure if my PMTU lookup works in
all cases (nested transforms?). Any pointer would be appreciated.
Martin Willi (5):
xfrm: Add Traffic Flow Confidentiality padding XFRM attribute
xfrm: Remove unused ESP padlen field
xfrm: Traffic Flow Confidentiality for IPv4 ESP
xfrm: Traffic Flow Confidentiality for IPv6 ESP
xfrm: Add TFC padding option to automatically pad to PMTU
include/linux/xfrm.h | 8 +++++++
include/net/esp.h | 3 --
include/net/xfrm.h | 1 +
net/ipv4/esp4.c | 58 +++++++++++++++++++++++++++++++++++--------------
net/ipv6/esp6.c | 58 +++++++++++++++++++++++++++++++++++--------------
net/xfrm/xfrm_user.c | 16 ++++++++++++-
6 files changed, 105 insertions(+), 39 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox