* Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
From: Ilpo Järvinen @ 2007-10-09 12:19 UTC (permalink / raw)
To: TAKANO Ryousei; +Cc: Netdev, y-kodama
In-Reply-To: <20071009.152834.38612348.takano@axe-inc.co.jp>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2629 bytes --]
On Tue, 9 Oct 2007, TAKANO Ryousei wrote:
> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Subject: Re: [RFC][PATCH 1/2] TCP: fix lost retransmit detection
> Date: Mon, 8 Oct 2007 14:11:55 +0300 (EEST)
>
> > On Sun, 7 Oct 2007, TAKANO Ryousei wrote:
> >
> > > From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> > >
> > > > Just couple of thoughts, not that this change itself is incorrect...
...Added this line back, it's important. :-)
> > > > In case sacktag uses fastpath, this code won't be executed for the skb's
> > > > that we would like to check (those with SACKED_RETRANS set, that are
> > > > below the fastpath_skb_hint). We will eventually deal with the whole queue
> > > > when fastpath_skb_hint gets set to NULL, with the next cumulative ACK that
> > > > fully ACKs an skb at the latest. Maybe there's a need for a larger surgery
> > > > than this to fix it. I think we need additional field to tcp_sock to avoid
> > > > doing a full-walk per ACK:
> > >
> > > I think the problem occurs in slowpath. For example, in case when the receiver
> > > detects and sends back a new SACK block, the sender may fail to detect loss
> > > of a retransmitted packet.
> >
> > ...No, the slow path is very likely to _correct_ the problem! The problem
> > occurs because fastpath _skips_ processing of skbs below fastpath_skb_hint
> > whereas slowpath starts from tcp_write_queue_head. The retransmitted skbs
> > we're interested in reside exactly on that skipped range.
> >
> Sorry, my explanaton is insufficient.
> This problem occurs even if we are in the slowpath.
>
> [...long explination snip...]
>
> Is it corrent?
Yes, we're dealing with two related, but different problems here. I
understood well what you're trying to fix, and even acknowledged your
change (see what I added back from my original reply above). The far
worse problem which I describe, however, occurs with fastpath only (well,
very unlikely to occur with slow-path but it could in theory), has been
like this since the hints were added. And it's preventing the execution
of this portion you fixed (until slowpath is taken).
...What makes the other problem even nastier is the fact that it
becomes more and more significant when fastpath (or it's equivalent)
skips more and more skb processing. It's good that we noticed it
now...
I tried to do a fix to that other problem, it seems that the solution
will be intertwined with the problem you're describing so that I in
fact ended removing the code block where your key modification is and
placing somewhat similar sequence gathering elsewhere... Posting it
shortly.
--
i.
^ permalink raw reply
* [RFC PATCH net-2.6.24 0/3]: Attempt to fix lost_retrans brokeness
From: Ilpo Järvinen @ 2007-10-09 12:19 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Stephen Hemminger, TAKANO Ryousei
Lost_retrans handling of sacktag was found to be flawed, two
problems that were found have an intertwined solution. Fastpath
problem has existed since hints got added and the other problem
has probably been there even longer than that. ...This change
may add non-trivial processing cost.
Initial sketch, only compile tested. This will become more and
more useful, when sacktag starts to process less and less skbs,
which hopefully happens quite soon... :-) Sadly enough it will
probably then be consuming part of the benefits we're able to
achieve by less skb walking...
First one is trivial, so Dave might want to apply it already.
--
i.
^ permalink raw reply
* [RFC PATCH] [TCP]: Fix lost_retrans loop vs fastpath problems
From: Ilpo Järvinen @ 2007-10-09 12:20 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Stephen Hemminger, TAKANO Ryousei
In-Reply-To: <11919324023846-git-send-email-ilpo.jarvinen@helsinki.fi>
Detection implemented with lost_retrans must work also when
fastpath is taken, yet most of the queue is skipped including
(very likely) those retransmitted skb's we're interested in.
This problem appeared when the hints got added, which removed
a need to always walk over the whole write queue head.
Therefore decicion for the lost_retrans worker loop entry must
be separated from the sacktag processing more than it was
necessary before.
It turns out to be problematic to optimize the worker loop
very heavily because ack_seqs of skb may have a number of
discontinuity points. Maybe similar approach as currently is
implemented could be attempted but that's becoming more and
more complex because the trend is towards less skb walking
in sacktag marker.
Maybe after(highest_sack_end_seq, tp->high_seq) checking is not
sufficiently accurate and causes entry too often in no-work-to-do
cases. Since that's not known, I've separated solution to that
from this patch.
Noticed because of report against a related problem from TAKANO
Ryousei <takano@axe-inc.co.jp>. He also provided a patch to
that part of the problem. This patch includes solution to it
(though this patch has to use somewhat different placement).
TAKANO's description and patch is available here:
http://marc.info/?l=linux-netdev&m=119149311913288&w=2
...In short, TAKANO's problem is that end_seq the loop is using
not necessarily the largest SACK block's end_seq because the
current ACK may still have higher SACK blocks which are later
by the loop.
Cc: TAKANO Ryousei <takano@axe-inc.co.jp>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
net/ipv4/tcp_input.c | 37 ++++++++++++++++++++++---------------
1 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 56aa34a..b90c2fc 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1109,27 +1109,34 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
/* Check for lost retransmit. This superb idea is borrowed from "ratehalving".
* Event "C". Later note: FACK people cheated me again 8), we have to account
* for reordering! Ugly, but should help.
+ *
+ * Search retransmitted skbs from write_queue that were sent when snd_nxt was
+ * less than what is now known to be received by the other end (derived from
+ * SACK blocks by the caller).
*/
-static int tcp_mark_lost_retrans(struct sock *sk, u32 lost_retrans)
+static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
{
struct tcp_sock *tp = tcp_sk(sk);
struct sk_buff *skb;
int flag = 0;
+ int cnt = 0;
tcp_for_write_queue(skb, sk) {
u32 ack_seq = TCP_SKB_CB(skb)->ack_seq;
if (skb == tcp_send_head(sk))
break;
- if (after(TCP_SKB_CB(skb)->seq, lost_retrans))
+ if (cnt == tp->retrans_out)
break;
if (!after(TCP_SKB_CB(skb)->end_seq, tp->snd_una))
continue;
- if ((TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS) &&
- after(lost_retrans, ack_seq) &&
+ if (!(TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS))
+ continue;
+
+ if (after(received_upto, ack_seq) &&
(tcp_is_fack(tp) ||
- !before(lost_retrans,
+ !before(received_upto,
ack_seq + tp->reordering * tp->mss_cache))) {
TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_RETRANS;
tp->retrans_out -= tcp_skb_pcount(skb);
@@ -1143,6 +1150,8 @@ static int tcp_mark_lost_retrans(struct sock *sk, u32 lost_retrans)
flag |= FLAG_DATA_SACKED;
NET_INC_STATS_BH(LINUX_MIB_TCPLOSTRETRANSMIT);
}
+ } else {
+ cnt += tcp_skb_pcount(skb);
}
}
return flag;
@@ -1193,7 +1202,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
int num_sacks = (ptr[1] - TCPOLEN_SACK_BASE)>>3;
int reord = tp->packets_out;
int prior_fackets;
- u32 lost_retrans = 0;
+ u32 highest_sack_end_seq = 0;
int flag = 0;
int found_dup_sack = 0;
int cached_fack_count;
@@ -1383,11 +1392,6 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
continue;
}
- if ((sacked&TCPCB_SACKED_RETRANS) &&
- after(end_seq, TCP_SKB_CB(skb)->ack_seq) &&
- (!lost_retrans || after(end_seq, lost_retrans)))
- lost_retrans = end_seq;
-
if (!in_sack)
continue;
@@ -1441,9 +1445,10 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
if (fack_count > tp->fackets_out)
tp->fackets_out = fack_count;
- if (after(TCP_SKB_CB(skb)->seq,
- tp->highest_sack))
+ if (after(TCP_SKB_CB(skb)->seq, tp->highest_sack)) {
tp->highest_sack = TCP_SKB_CB(skb)->seq;
+ highest_sack_end_seq = TCP_SKB_CB(skb)->end_seq;
+ }
} else {
if (dup_sack && (sacked&TCPCB_RETRANS))
reord = min(fack_count, reord);
@@ -1463,8 +1468,10 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
}
}
- if (lost_retrans && icsk->icsk_ca_state == TCP_CA_Recovery)
- flag |= tcp_mark_lost_retrans(sk, lost_retrans);
+ if (tp->retrans_out && highest_sack_end_seq &&
+ after(highest_sack_end_seq, tp->high_seq) &&
+ icsk->icsk_ca_state == TCP_CA_Recovery)
+ flag |= tcp_mark_lost_retrans(sk, highest_sack_end_seq);
tcp_verify_left_out(tp);
--
1.5.0.6
^ permalink raw reply related
* [PATCH] [TCP]: Separate lost_retrans loop into own function
From: Ilpo Järvinen @ 2007-10-09 12:20 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Stephen Hemminger, TAKANO Ryousei
In-Reply-To: <11919324023597-git-send-email-ilpo.jarvinen@helsinki.fi>
Follows own function for each task principle, this is really
somewhat separate task being done in sacktag. Also reduces
indentation.
In addition, added ack_seq local var to break some long
lines & fixed coding style things.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
net/ipv4/tcp_input.c | 80 +++++++++++++++++++++++++++-----------------------
1 files changed, 43 insertions(+), 37 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a04e78e..56aa34a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1106,6 +1106,47 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
return !before(start_seq, end_seq - tp->max_window);
}
+/* Check for lost retransmit. This superb idea is borrowed from "ratehalving".
+ * Event "C". Later note: FACK people cheated me again 8), we have to account
+ * for reordering! Ugly, but should help.
+ */
+static int tcp_mark_lost_retrans(struct sock *sk, u32 lost_retrans)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+ struct sk_buff *skb;
+ int flag = 0;
+
+ tcp_for_write_queue(skb, sk) {
+ u32 ack_seq = TCP_SKB_CB(skb)->ack_seq;
+
+ if (skb == tcp_send_head(sk))
+ break;
+ if (after(TCP_SKB_CB(skb)->seq, lost_retrans))
+ break;
+ if (!after(TCP_SKB_CB(skb)->end_seq, tp->snd_una))
+ continue;
+
+ if ((TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS) &&
+ after(lost_retrans, ack_seq) &&
+ (tcp_is_fack(tp) ||
+ !before(lost_retrans,
+ ack_seq + tp->reordering * tp->mss_cache))) {
+ TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_RETRANS;
+ tp->retrans_out -= tcp_skb_pcount(skb);
+
+ /* clear lost hint */
+ tp->retransmit_skb_hint = NULL;
+
+ if (!(TCP_SKB_CB(skb)->sacked & (TCPCB_LOST|TCPCB_SACKED_ACKED))) {
+ tp->lost_out += tcp_skb_pcount(skb);
+ TCP_SKB_CB(skb)->sacked |= TCPCB_LOST;
+ flag |= FLAG_DATA_SACKED;
+ NET_INC_STATS_BH(LINUX_MIB_TCPLOSTRETRANSMIT);
+ }
+ }
+ }
+ return flag;
+}
static int tcp_check_dsack(struct tcp_sock *tp, struct sk_buff *ack_skb,
struct tcp_sack_block_wire *sp, int num_sacks,
@@ -1422,43 +1463,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
}
}
- /* Check for lost retransmit. This superb idea is
- * borrowed from "ratehalving". Event "C".
- * Later note: FACK people cheated me again 8),
- * we have to account for reordering! Ugly,
- * but should help.
- */
- if (lost_retrans && icsk->icsk_ca_state == TCP_CA_Recovery) {
- struct sk_buff *skb;
-
- tcp_for_write_queue(skb, sk) {
- if (skb == tcp_send_head(sk))
- break;
- if (after(TCP_SKB_CB(skb)->seq, lost_retrans))
- break;
- if (!after(TCP_SKB_CB(skb)->end_seq, tp->snd_una))
- continue;
- if ((TCP_SKB_CB(skb)->sacked&TCPCB_SACKED_RETRANS) &&
- after(lost_retrans, TCP_SKB_CB(skb)->ack_seq) &&
- (tcp_is_fack(tp) ||
- !before(lost_retrans,
- TCP_SKB_CB(skb)->ack_seq + tp->reordering *
- tp->mss_cache))) {
- TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_RETRANS;
- tp->retrans_out -= tcp_skb_pcount(skb);
-
- /* clear lost hint */
- tp->retransmit_skb_hint = NULL;
-
- if (!(TCP_SKB_CB(skb)->sacked&(TCPCB_LOST|TCPCB_SACKED_ACKED))) {
- tp->lost_out += tcp_skb_pcount(skb);
- TCP_SKB_CB(skb)->sacked |= TCPCB_LOST;
- flag |= FLAG_DATA_SACKED;
- NET_INC_STATS_BH(LINUX_MIB_TCPLOSTRETRANSMIT);
- }
- }
- }
- }
+ if (lost_retrans && icsk->icsk_ca_state == TCP_CA_Recovery)
+ flag |= tcp_mark_lost_retrans(sk, lost_retrans);
tcp_verify_left_out(tp);
--
1.5.0.6
^ permalink raw reply related
* [RFC PATCH] [TCP]: Limit processing lost_retrans loop to work-to-do cases
From: Ilpo Järvinen @ 2007-10-09 12:20 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Stephen Hemminger, TAKANO Ryousei
In-Reply-To: <119193240269-git-send-email-ilpo.jarvinen@helsinki.fi>
This addition of lost_retrans_low to tcp_sock might be
unnecessary, it's not clear how often lost_retrans worker is
executed when there wasn't work to do.
Cc: TAKANO Ryousei <takano@axe-inc.co.jp>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
include/linux/tcp.h | 2 ++
net/ipv4/tcp_input.c | 15 +++++++++++----
net/ipv4/tcp_output.c | 2 ++
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 9ff456e..c5b94c1 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -348,6 +348,8 @@ struct tcp_sock {
int lost_cnt_hint;
int retransmit_cnt_hint;
+ u32 lost_retrans_low; /* Sent seq after any rxmit (lowest) */
+
u16 advmss; /* Advertised MSS */
u16 prior_ssthresh; /* ssthresh saved at recovery start */
u32 lost_out; /* Lost packets */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b90c2fc..e7bc720 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1112,7 +1112,8 @@ static int tcp_is_sackblock_valid(struct tcp_sock *tp, int is_dsack,
*
* Search retransmitted skbs from write_queue that were sent when snd_nxt was
* less than what is now known to be received by the other end (derived from
- * SACK blocks by the caller).
+ * SACK blocks by the caller). Also calculate the lowest snd_nxt among the
+ * remaining retransmitted skbs to avoid some costly processing per ACKs.
*/
static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
{
@@ -1120,6 +1121,7 @@ static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
struct sk_buff *skb;
int flag = 0;
int cnt = 0;
+ u32 new_low_seq = 0;
tcp_for_write_queue(skb, sk) {
u32 ack_seq = TCP_SKB_CB(skb)->ack_seq;
@@ -1150,10 +1152,15 @@ static int tcp_mark_lost_retrans(struct sock *sk, u32 received_upto)
flag |= FLAG_DATA_SACKED;
NET_INC_STATS_BH(LINUX_MIB_TCPLOSTRETRANSMIT);
}
- } else {
+ } else if (!new_low_seq || before(ack_seq, new_low_seq)) {
+ new_low_seq = ack_seq;
cnt += tcp_skb_pcount(skb);
}
}
+
+ if (tp->retrans_out)
+ tp->lost_retrans_low = new_low_seq;
+
return flag;
}
@@ -1468,8 +1475,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb, u32 prior_snd_
}
}
- if (tp->retrans_out && highest_sack_end_seq &&
- after(highest_sack_end_seq, tp->high_seq) &&
+ if (tp->retrans_out &&
+ after(highest_sack_end_seq, tp->lost_retrans_low) &&
icsk->icsk_ca_state == TCP_CA_Recovery)
flag |= tcp_mark_lost_retrans(sk, highest_sack_end_seq);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 5329675..324b420 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1914,6 +1914,8 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
printk(KERN_DEBUG "retrans_out leaked.\n");
}
#endif
+ if (!tp->retrans_out)
+ tp->lost_retrans_low = tp->snd_nxt;
TCP_SKB_CB(skb)->sacked |= TCPCB_RETRANS;
tp->retrans_out += tcp_skb_pcount(skb);
--
1.5.0.6
^ permalink raw reply related
* [PATCH][NETNS] Make ifindex generation per-namespace
From: Pavel Emelyanov @ 2007-10-09 12:19 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List, devel, Eric W. Biederman
Currently indexes for netdevices come sequentially one by
one, and the same stays true even for devices that are
created for namespaces.
Side effects of this are:
* lo device has not 1 index in a namespace. This may break
some userspace that relies on it (and AFAIR something
really broke in OpenVZ VEs without this);
* after some time namespaces will have devices with indexes
like 1000000 os similar. This might be confusing for a
human (tools will not mind).
So move the (currently "global" and static) ifindex variable
on the struct net, making the indexes allocation look more
like on a standalone machine.
Moreover - when we have indexes intersect between namespaces,
we may catch more BUGs in the future related to "wrong device
was found for a given index".
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 93aa87d..83a18d0 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -29,6 +29,8 @@ struct net {
struct list_head dev_base_head;
struct hlist_head *dev_name_head;
struct hlist_head *dev_index_head;
+
+ int ifindex;
};
#ifdef CONFIG_NET
diff --git a/net/core/dev.c b/net/core/dev.c
index e7e728a..a08ed8c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3443,12 +3443,11 @@ int dev_ioctl(struct net *net, unsigned
*/
static int dev_new_index(struct net *net)
{
- static int ifindex;
for (;;) {
- if (++ifindex <= 0)
- ifindex = 1;
- if (!__dev_get_by_index(net, ifindex))
- return ifindex;
+ if (++net->ifindex <= 0)
+ net->ifindex = 1;
+ if (!__dev_get_by_index(net, net->ifindex))
+ return net->ifindex;
}
}
^ permalink raw reply related
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Jeff Garzik @ 2007-10-09 12:44 UTC (permalink / raw)
To: David Miller
Cc: jagana, herbert, gaagaan, Robert.Olsson, mcarlson, rdreier,
peter.p.waskiewicz.jr, hadi, netdev, general, mchan, tgraf,
randy.dunlap, johnpol, shemminger, kaber, sri
In-Reply-To: <20071009.042441.30182968.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 678 bytes --]
David Miller wrote:
> From: Krishna Kumar2 <krkumar2@in.ibm.com>
> Date: Tue, 9 Oct 2007 16:51:14 +0530
>
>> David Miller <davem@davemloft.net> wrote on 10/09/2007 04:32:55 PM:
>>
>>> Ignore LLTX, it sucks, it was a big mistake, and we will get rid of
>>> it.
>> Great, this will make life easy. Any idea how long that would take?
>> It seems simple enough to do.
>
> I'd say we can probably try to get rid of it in 2.6.25, this is
> assuming we get driver authors to cooperate and do the conversions
> or alternatively some other motivated person.
>
> I can just threaten to do them all and that should get the driver
> maintainers going :-)
What, like this? :)
Jeff
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 13349 bytes --]
drivers/net/atl1/atl1_main.c | 16 +++++-----------
drivers/net/chelsio/cxgb2.c | 1 -
drivers/net/chelsio/sge.c | 20 +++++++++-----------
drivers/net/e1000/e1000_main.c | 6 +-----
drivers/net/ixgb/ixgb_main.c | 24 ------------------------
drivers/net/pasemi_mac.c | 2 +-
drivers/net/rionet.c | 19 +++++++------------
drivers/net/spider_net.c | 2 +-
drivers/net/sungem.c | 17 ++++++-----------
drivers/net/tehuti.c | 12 +-----------
drivers/net/tehuti.h | 3 +--
11 files changed, 32 insertions(+), 90 deletions(-)
diff --git a/drivers/net/atl1/atl1_main.c b/drivers/net/atl1/atl1_main.c
index 4c728f1..03e94fe 100644
--- a/drivers/net/atl1/atl1_main.c
+++ b/drivers/net/atl1/atl1_main.c
@@ -1665,10 +1665,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
len -= skb->data_len;
- if (unlikely(skb->len == 0)) {
- dev_kfree_skb_any(skb);
- return NETDEV_TX_OK;
- }
+ WARN_ON(skb->len == 0);
param.data = 0;
param.tso.tsopu = 0;
@@ -1703,11 +1700,7 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
}
}
- if (!spin_trylock_irqsave(&adapter->lock, flags)) {
- /* Can't get lock - tell upper layer to requeue */
- dev_printk(KERN_DEBUG, &adapter->pdev->dev, "tx locked\n");
- return NETDEV_TX_LOCKED;
- }
+ spin_lock_irqsave(&adapter->lock, flags);
if (atl1_tpd_avail(&adapter->tpd_ring) < count) {
/* not enough descriptors */
@@ -1749,8 +1742,11 @@ static int atl1_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
atl1_tx_map(adapter, skb, 1 == val);
atl1_tx_queue(adapter, count, ¶m);
netdev->trans_start = jiffies;
+
spin_unlock_irqrestore(&adapter->lock, flags);
+
atl1_update_mailbox(adapter);
+
return NETDEV_TX_OK;
}
@@ -2301,8 +2297,6 @@ static int __devinit atl1_probe(struct pci_dev *pdev,
*/
/* netdev->features |= NETIF_F_TSO; */
- netdev->features |= NETIF_F_LLTX;
-
/*
* patch for some L1 of old version,
* the final version of L1 may not need these
diff --git a/drivers/net/chelsio/cxgb2.c b/drivers/net/chelsio/cxgb2.c
index 2dbf8dc..0aba7e7 100644
--- a/drivers/net/chelsio/cxgb2.c
+++ b/drivers/net/chelsio/cxgb2.c
@@ -1084,7 +1084,6 @@ static int __devinit init_one(struct pci_dev *pdev,
netdev->mem_end = mmio_start + mmio_len - 1;
netdev->priv = adapter;
netdev->features |= NETIF_F_SG | NETIF_F_IP_CSUM;
- netdev->features |= NETIF_F_LLTX;
adapter->flags |= RX_CSUM_ENABLED | TCP_CSUM_CAPABLE;
if (pci_using_dac)
diff --git a/drivers/net/chelsio/sge.c b/drivers/net/chelsio/sge.c
index ffa7e64..84f5869 100644
--- a/drivers/net/chelsio/sge.c
+++ b/drivers/net/chelsio/sge.c
@@ -1739,8 +1739,7 @@ static int t1_sge_tx(struct sk_buff *skb, struct adapter *adapter,
struct cmdQ *q = &sge->cmdQ[qid];
unsigned int credits, pidx, genbit, count, use_sched_skb = 0;
- if (!spin_trylock(&q->lock))
- return NETDEV_TX_LOCKED;
+ spin_lock(&q->lock);
reclaim_completed_tx(sge, q);
@@ -1817,12 +1816,12 @@ use_sched:
}
if (use_sched_skb) {
- if (spin_trylock(&q->lock)) {
- credits = q->size - q->in_use;
- skb = NULL;
- goto use_sched;
- }
+ spin_lock(&q->lock);
+ credits = q->size - q->in_use;
+ skb = NULL;
+ goto use_sched;
}
+
return NETDEV_TX_OK;
}
@@ -1977,13 +1976,12 @@ static void sge_tx_reclaim_cb(unsigned long data)
for (i = 0; i < SGE_CMDQ_N; ++i) {
struct cmdQ *q = &sge->cmdQ[i];
- if (!spin_trylock(&q->lock))
- continue;
+ spin_lock(&q->lock);
reclaim_completed_tx(sge, q);
- if (i == 0 && q->in_use) { /* flush pending credits */
+ if (i == 0 && q->in_use) /* flush pending credits */
writel(F_CMDQ0_ENABLE, sge->adapter->regs + A_SG_DOORBELL);
- }
+
spin_unlock(&q->lock);
}
mod_timer(&sge->tx_reclaim_timer, jiffies + TX_RECLAIM_PERIOD);
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 10505de..b64b03b 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -992,8 +992,6 @@ e1000_probe(struct pci_dev *pdev,
if (pci_using_dac)
netdev->features |= NETIF_F_HIGHDMA;
- netdev->features |= NETIF_F_LLTX;
-
adapter->en_mng_pt = e1000_enable_mng_pass_thru(&adapter->hw);
/* initialize eeprom parameters */
@@ -3368,9 +3366,7 @@ e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
(adapter->hw.mac_type == e1000_82573))
e1000_transfer_dhcp_info(adapter, skb);
- if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
- /* Collision - tell upper layer to requeue */
- return NETDEV_TX_LOCKED;
+ spin_lock_irqsave(&tx_ring->tx_lock, flags);
/* need: count + 2 desc gap to keep tail from touching
* head, otherwise try next time */
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index d444de5..c04b59b 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -449,9 +449,6 @@ ixgb_probe(struct pci_dev *pdev,
NETIF_F_HW_VLAN_RX |
NETIF_F_HW_VLAN_FILTER;
netdev->features |= NETIF_F_TSO;
-#ifdef NETIF_F_LLTX
- netdev->features |= NETIF_F_LLTX;
-#endif
if(pci_using_dac)
netdev->features |= NETIF_F_HIGHDMA;
@@ -1455,16 +1452,7 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
return 0;
}
-#ifdef NETIF_F_LLTX
- local_irq_save(flags);
- if (!spin_trylock(&adapter->tx_lock)) {
- /* Collision - tell upper layer to requeue */
- local_irq_restore(flags);
- return NETDEV_TX_LOCKED;
- }
-#else
spin_lock_irqsave(&adapter->tx_lock, flags);
-#endif
if (unlikely(ixgb_maybe_stop_tx(netdev, &adapter->tx_ring,
DESC_NEEDED))) {
@@ -1473,9 +1461,7 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
return NETDEV_TX_BUSY;
}
-#ifndef NETIF_F_LLTX
spin_unlock_irqrestore(&adapter->tx_lock, flags);
-#endif
if(adapter->vlgrp && vlan_tx_tag_present(skb)) {
tx_flags |= IXGB_TX_FLAGS_VLAN;
@@ -1487,9 +1473,6 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
tso = ixgb_tso(adapter, skb);
if (tso < 0) {
dev_kfree_skb_any(skb);
-#ifdef NETIF_F_LLTX
- spin_unlock_irqrestore(&adapter->tx_lock, flags);
-#endif
return NETDEV_TX_OK;
}
@@ -1503,13 +1486,6 @@ ixgb_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
netdev->trans_start = jiffies;
-#ifdef NETIF_F_LLTX
- /* Make sure there is space in the ring for the next send. */
- ixgb_maybe_stop_tx(netdev, &adapter->tx_ring, DESC_NEEDED);
-
- spin_unlock_irqrestore(&adapter->tx_lock, flags);
-
-#endif
return NETDEV_TX_OK;
}
diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index 9f9a421..78f939b 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -1352,7 +1352,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
netif_napi_add(dev, &mac->napi, pasemi_mac_poll, 64);
- dev->features = NETIF_F_HW_CSUM | NETIF_F_LLTX | NETIF_F_SG;
+ dev->features = NETIF_F_HW_CSUM | NETIF_F_SG;
/* These should come out of the device tree eventually */
mac->dma_txch = index;
diff --git a/drivers/net/rionet.c b/drivers/net/rionet.c
index e7fd08a..cd2d25c 100644
--- a/drivers/net/rionet.c
+++ b/drivers/net/rionet.c
@@ -180,11 +180,7 @@ static int rionet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
u16 destid;
unsigned long flags;
- local_irq_save(flags);
- if (!spin_trylock(&rnet->tx_lock)) {
- local_irq_restore(flags);
- return NETDEV_TX_LOCKED;
- }
+ spin_lock_irqsave(&rnet->tx_lock, flags);
if ((rnet->tx_cnt + 1) > RIONET_TX_RING_SIZE) {
netif_stop_queue(ndev);
@@ -259,7 +255,7 @@ static void rionet_outb_msg_event(struct rio_mport *mport, void *dev_id, int mbo
struct net_device *ndev = dev_id;
struct rionet_private *rnet = ndev->priv;
- spin_lock(&rnet->lock);
+ spin_lock(&rnet->tx_lock);
if (netif_msg_intr(rnet))
printk(KERN_INFO
@@ -278,7 +274,7 @@ static void rionet_outb_msg_event(struct rio_mport *mport, void *dev_id, int mbo
if (rnet->tx_cnt < RIONET_TX_RING_SIZE)
netif_wake_queue(ndev);
- spin_unlock(&rnet->lock);
+ spin_unlock(&rnet->tx_lock);
}
static int rionet_open(struct net_device *ndev)
@@ -420,10 +416,10 @@ static void rionet_set_msglevel(struct net_device *ndev, u32 value)
}
static const struct ethtool_ops rionet_ethtool_ops = {
- .get_drvinfo = rionet_get_drvinfo,
- .get_msglevel = rionet_get_msglevel,
- .set_msglevel = rionet_set_msglevel,
- .get_link = ethtool_op_get_link,
+ .get_drvinfo = rionet_get_drvinfo,
+ .get_msglevel = rionet_get_msglevel,
+ .set_msglevel = rionet_set_msglevel,
+ .get_link = ethtool_op_get_link,
};
static int rionet_setup_netdev(struct rio_mport *mport)
@@ -461,7 +457,6 @@ static int rionet_setup_netdev(struct rio_mport *mport)
ndev->hard_start_xmit = &rionet_start_xmit;
ndev->stop = &rionet_close;
ndev->mtu = RIO_MAX_MSG_SIZE - 14;
- ndev->features = NETIF_F_LLTX;
SET_ETHTOOL_OPS(ndev, &rionet_ethtool_ops);
spin_lock_init(&rnet->lock);
diff --git a/drivers/net/spider_net.c b/drivers/net/spider_net.c
index fab055f..22193a9 100644
--- a/drivers/net/spider_net.c
+++ b/drivers/net/spider_net.c
@@ -2329,7 +2329,7 @@ spider_net_setup_netdev(struct spider_net_card *card)
spider_net_setup_netdev_ops(netdev);
- netdev->features = NETIF_F_IP_CSUM | NETIF_F_LLTX;
+ netdev->features = NETIF_F_IP_CSUM;
/* some time: NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX |
* NETIF_F_HW_VLAN_FILTER */
diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 53b8344..6aaf56d 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -932,7 +932,6 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
{
struct net_device *dev = dev_id;
struct gem *gp = dev->priv;
- unsigned long flags;
/* Swallow interrupts when shutting the chip down, though
* that shouldn't happen, we should have done free_irq() at
@@ -941,14 +940,14 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
if (!gp->running)
return IRQ_HANDLED;
- spin_lock_irqsave(&gp->lock, flags);
+ spin_lock(&gp->lock);
if (netif_rx_schedule_prep(dev, &gp->napi)) {
u32 gem_status = readl(gp->regs + GREG_STAT);
if (gem_status == 0) {
napi_enable(&gp->napi);
- spin_unlock_irqrestore(&gp->lock, flags);
+ spin_unlock(&gp->lock);
return IRQ_NONE;
}
gp->status = gem_status;
@@ -956,7 +955,7 @@ static irqreturn_t gem_interrupt(int irq, void *dev_id)
__netif_rx_schedule(dev, &gp->napi);
}
- spin_unlock_irqrestore(&gp->lock, flags);
+ spin_unlock(&gp->lock);
/* If polling was disabled at the time we received that
* interrupt, we may return IRQ_HANDLED here while we
@@ -1031,12 +1030,8 @@ static int gem_start_xmit(struct sk_buff *skb, struct net_device *dev)
(csum_stuff_off << 21));
}
- local_irq_save(flags);
- if (!spin_trylock(&gp->tx_lock)) {
- /* Tell upper layer to requeue */
- local_irq_restore(flags);
- return NETDEV_TX_LOCKED;
- }
+ spin_lock_irqsave(&gp->tx_lock, flags);
+
/* We raced with gem_do_stop() */
if (!gp->running) {
spin_unlock_irqrestore(&gp->tx_lock, flags);
@@ -3160,7 +3155,7 @@ static int __devinit gem_init_one(struct pci_dev *pdev,
gp->phy_mii.def ? gp->phy_mii.def->name : "no");
/* GEM can do it all... */
- dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_LLTX;
+ dev->features |= NETIF_F_SG | NETIF_F_HW_CSUM;
if (pci_using_dac)
dev->features |= NETIF_F_HIGHDMA;
diff --git a/drivers/net/tehuti.c b/drivers/net/tehuti.c
index 2483431..e0da1e0 100644
--- a/drivers/net/tehuti.c
+++ b/drivers/net/tehuti.c
@@ -1606,7 +1606,6 @@ static inline int bdx_tx_space(struct bdx_priv *priv)
* o NETDEV_TX_BUSY Cannot transmit packet, try later
* Usually a bug, means queue start/stop flow control is broken in
* the driver. Note: the driver must NOT put the skb in its DMA ring.
- * o NETDEV_TX_LOCKED Locking failed, please retry quickly.
*/
static int bdx_tx_transmit(struct sk_buff *skb, struct net_device *ndev)
{
@@ -1624,13 +1623,7 @@ static int bdx_tx_transmit(struct sk_buff *skb, struct net_device *ndev)
unsigned long flags;
ENTER;
- local_irq_save(flags);
- if (!spin_trylock(&priv->tx_lock)) {
- local_irq_restore(flags);
- DBG("%s[%s]: TX locked, returning NETDEV_TX_LOCKED\n",
- BDX_DRV_NAME, ndev->name);
- return NETDEV_TX_LOCKED;
- }
+ spin_lock_irqsave(&priv->tx_lock, flags);
/* build tx descriptor */
BDX_ASSERT(f->m.wptr >= f->m.memsz); /* started with valid wptr */
@@ -2048,9 +2041,6 @@ bdx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
* between transmit and TX irq cleanup. In addition
* set multicast list callback has to use priv->tx_lock.
*/
-#ifdef BDX_LLTX
- ndev->features |= NETIF_F_LLTX;
-#endif
spin_lock_init(&priv->tx_lock);
/*bdx_hw_reset(priv); */
diff --git a/drivers/net/tehuti.h b/drivers/net/tehuti.h
index efd170f..c55f69a 100644
--- a/drivers/net/tehuti.h
+++ b/drivers/net/tehuti.h
@@ -35,7 +35,6 @@
/* Compile Time Switches */
/* start */
#define BDX_TSO
-#define BDX_LLTX
#define BDX_DELAY_WPTR
/* #define BDX_MSI */
/* end */
@@ -270,7 +269,7 @@ struct bdx_priv {
int tx_update_mark;
int tx_noupd;
#endif
- spinlock_t tx_lock; /* NETIF_F_LLTX mode */
+ spinlock_t tx_lock;
/* rarely used */
u8 port;
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related
* ax88796: add superh to kconfig depencencies
From: Magnus Damm @ 2007-10-09 12:51 UTC (permalink / raw)
To: netdev; +Cc: lethal, linuxsh-dev
ax88796: add superh to kconfig depencencies
This patch adds sh architecture support to the ax88796 kconfig.
Signed-off-by: Magnus Damm <damm@igel.co.jp>
---
This is a broken out version of the larger patch recently posted to netdev:
http://www.mail-archive.com/netdev@vger.kernel.org/msg47278.html
drivers/net/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- 0002/drivers/net/Kconfig
+++ work/drivers/net/Kconfig 2007-10-09 19:37:27.000000000 +0900
@@ -218,7 +218,7 @@ source "drivers/net/arm/Kconfig"
config AX88796
tristate "ASIX AX88796 NE2000 clone support"
- depends on ARM || MIPS
+ depends on ARM || MIPS || SUPERH
select CRC32
select MII
help
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Herbert Xu @ 2007-10-09 12:55 UTC (permalink / raw)
To: Jeff Garzik
Cc: jagana, gaagaan, Robert.Olsson, mcarlson, rdreier,
peter.p.waskiewicz.jr, hadi, kaber, netdev, general, mchan, tgraf,
randy.dunlap, johnpol, shemminger, David Miller, sri
In-Reply-To: <470B77A9.600@garzik.org>
On Tue, Oct 09, 2007 at 08:44:25AM -0400, Jeff Garzik wrote:
> David Miller wrote:
> >
> >I can just threaten to do them all and that should get the driver
> >maintainers going :-)
>
> What, like this? :)
Awsome :)
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 2/3][NET_BATCH] net core use batching
From: Jeff Garzik @ 2007-10-09 13:00 UTC (permalink / raw)
To: Herbert Xu
Cc: David Miller, krkumar2, gaagaan, general, hadi, jagana, johnpol,
kaber, mcarlson, mchan, netdev, peter.p.waskiewicz.jr,
randy.dunlap, rdreier, rick.jones2, Robert.Olsson, shemminger,
sri, tgraf, xma
In-Reply-To: <20071009125513.GA19650@gondor.apana.org.au>
Herbert Xu wrote:
> On Tue, Oct 09, 2007 at 08:44:25AM -0400, Jeff Garzik wrote:
>> David Miller wrote:
>>> I can just threaten to do them all and that should get the driver
>>> maintainers going :-)
>> What, like this? :)
>
> Awsome :)
Note my patch is just to get the maintainers going. :) I'm not going
to commit that, since I don't have any way to test any of the drivers I
touched (but I wouldn't scream if it appeared in net-2.6.24 either)
Jeff
^ permalink raw reply
* Re: [RFC PATCH net-2.6.24 0/3]: Attempt to fix lost_retrans brokeness
From: Ilpo Järvinen @ 2007-10-09 13:03 UTC (permalink / raw)
To: Netdev; +Cc: David Miller, Stephen Hemminger, TAKANO Ryousei
In-Reply-To: <11919324023597-git-send-email-ilpo.jarvinen@helsinki.fi>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1261 bytes --]
On Tue, 9 Oct 2007, Ilpo Järvinen wrote:
> Lost_retrans handling of sacktag was found to be flawed, two
> problems that were found have an intertwined solution. Fastpath
> problem has existed since hints got added and the other problem
> has probably been there even longer than that. ...This change
> may add non-trivial processing cost.
>
> Initial sketch, only compile tested. This will become more and
> more useful, when sacktag starts to process less and less skbs,
> which hopefully happens quite soon... :-) Sadly enough it will
> probably then be consuming part of the benefits we're able to
> achieve by less skb walking...
>
> First one is trivial, so Dave might want to apply it already.
Hmm, forgot to add -n to git-format-patch. Since it's currently
RFC, I won't bother to resubmit with numbers unless somebody
really wants that. Here's the correct ordering, if it's not
obvious from the patches alone:
$ git-log -n 3 --pretty=oneline HEAD | cat
c628f5040cf31323f9166d41cb46070aa7be8cc5 [TCP]: Limit processing lost_retrans loop to work-to-do cases
b7388980b7bfa1b9159a65a81b9501df43c67b08 [TCP]: Fix lost_retrans loop vs fastpath problems
a1c39117034c58bf786a5941dd7e9a5968439007 [TCP]: Separate lost_retrans loop into own function
--
i.
^ permalink raw reply
* [ofa-general] Re: [PATCH 2/3][NET_BATCH] net core use batching
From: jamal @ 2007-10-09 13:10 UTC (permalink / raw)
To: Krishna Kumar2
Cc: jagana, johnpol, gaagaan, jeff, Robert.Olsson, kumarkr, rdreier,
peter.p.waskiewicz.jr, mcarlson, Patrick McHardy, netdev, general,
mchan, tgraf, randy.dunlap, sri, shemminger, David Miller,
herbert
In-Reply-To: <OFDE12F216.8E927E22-ON6525736F.001137D0-6525736F.00115A01@in.ibm.com>
On Tue, 2007-09-10 at 08:39 +0530, Krishna Kumar2 wrote:
> Driver might ask for 10 and we send 10, but LLTX driver might fail to get
> lock and return TX_LOCKED. I haven't seen your code in greater detail, but
> don't you requeue in that case too?
For others drivers that are non-batching and LLTX, it is possible - at
the moment in my patch i whine that the driver is buggy. I will fix this
up so it checks for NETIF_F_BTX. Thanks for pointing the above use case.
cheers,
jamal
^ permalink raw reply
* Re: [PATCHES] TX batching
From: jamal @ 2007-10-09 13:25 UTC (permalink / raw)
To: Krishna Kumar2
Cc: David Miller, gaagaan, general, herbert, jagana, jeff,
Evgeniy Polyakov, kaber, kumarkr, mcarlson, mchan, netdev,
peter.p.waskiewicz.jr, randy.dunlap, rdreier, rick.jones2,
Robert.Olsson, shemminger, sri, tgraf, xma
In-Reply-To: <OF568108F3.5706A2B5-ON6525736F.002CED41-6525736F.002D492F@in.ibm.com>
On Tue, 2007-09-10 at 13:44 +0530, Krishna Kumar2 wrote:
> My feeling is that since the approaches are very different,
My concern is the approaches are different only for short periods of
time. For example, I do requeueing, have xmit_win, have ->end_xmit,
do batching from core etc; if you see value in any of these concepts,
they will appear in your patches and this goes on a loop. Perhaps what
we need is a referee and use our energies in something more positive.
> it would be a good idea to test the two for performance.
Which i dont mind as long as it has an analysis that goes with it.
If all you post is "heres what netperf showed", it is not useful at all.
There are also a lot of affecting variables. For example, is the
receiver a bottleneck? To make it worse, I could demonstrate to you that
if i slowed down the driver and allowed more packets to queue up on the
qdisc, batching will do well. In the past my feeling is you glossed over
such details and i am sucker for things like that - hence the conflict.
> Do you mind me doing
> that? Ofcourse others and/or you are more than welcome to do the same.
>
> I had sent a note to you yesterday about this, please let me know
> either way.
>
I responded to you - but it may have been lost in the noise; heres a
copy:
http://marc.info/?l=linux-netdev&m=119185137124008&w=2
cheers,
jamal
^ permalink raw reply
* Re: [PATCH][E1000E] some cleanups
From: jamal @ 2007-10-09 13:29 UTC (permalink / raw)
To: Kok, Auke; +Cc: netdev
In-Reply-To: <470AB1EC.2080000@intel.com>
On Mon, 2007-08-10 at 15:40 -0700, Kok, Auke wrote:
>
> My biggest problem with the patch as you sent it that it's a tonload of changes
> and no implicit benefit immediately as I can see.
The patch looks scary but is pretty tame when you apply it and stare at
it.
> I would really have to see the
> LLTX change as well to make a wellfounded decision whether it is the right thing
> to go and overhaul this part of the driver or not :)
>
> I'm not particularly against the changes per se though.
>
> so please, if needed offlist send me those LLTX changes as well.
Dont worry about it - i didnt get any love for a similar patch i did for
tg3 either ;-> I will hold onto it for now. I could do the un-LLTX
outside of this - would you like that for both e1000/e1000e ?
cheers,
jamal
^ permalink raw reply
* Re: [RFC][NET_SCHED] explict hold dev tx lock
From: jamal @ 2007-10-09 13:43 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, kaber, dada1, johnpol
In-Reply-To: <20071009040037.GA15981@gondor.apana.org.au>
On Tue, 2007-09-10 at 12:00 +0800, Herbert Xu wrote:
>
> OK, after waking up a bit more
me too;->
> What I'm worried about is would we see worse behaviour with
> drivers that do all their TX clean-up with the TX lock held
Good point Herbert.
When i looked around i only found one driver that behaved like that;
some IBM mainframe one from the looks of it. That driver did a lot of
other obscure things (i think they maintain their own napi calls etc),
so it didnt worry me very much.
IIRC, I think it could be fixed to do what tg3 and relatives like bnx do
(I really like the approach) - just didnt have the time to chase it.
There are a _lot_ more drivers that have no respect for netif_tx_lock
and implement their own locking down in the driver. Those already suffer
from the phenomena you describe whether TX_LOCK is held or not.
> (which would cause qdisc_restart to spin while this is happening)?
Yes, with such a driver, we spin in the worst case. But that provides
opportunities for optimization of a driver behaving that way; two
approaches off top of my head:
a) we could prune the tx descriptors on the the tx path since it is safe
to do so and reduce the amount of time spent doing such work in the napi
poll
or
b) Have the napi side do a trylock (sort of what the e1000 attempts to
do) and reschedule the poll to retry.
#b is fair because the cost of a queue_lock goes up as the number of
cpus goes up (which is the case i was optimizing for). The cost of tx
lock is contention by two cpus in worst case. Thoughts?
cheers,
jamal
^ permalink raw reply
* Re: [ofa-general] Re: [PATCH RFC] RDMA/CMA: Allocate PS_TCP ports from the host TCP port space.
From: James Lentini @ 2007-10-09 13:44 UTC (permalink / raw)
To: Steve Wise; +Cc: David Miller, rdreier, linux-kernel, general, netdev
In-Reply-To: <470AA729.2050009@opengridcomputing.com>
On Mon, 8 Oct 2007, Steve Wise wrote:
> The correct solution, IMO, is to enhance the core low level 4-tuple
> allocation services to be more generic (eg: not be tied to a struct
> sock). Then the host tcp stack and the host rdma stack can allocate
> TCP/iWARP ports/4tuples from this common exported service and share
> the port space. This allocation service could also be used by other
> deep adapters like iscsi adapters if needed.
As a developer of an RDMA ULP, NFS-RDMA, I like this approach because
it will simplify the configuration of an RDMA device and the services
that use it.
^ permalink raw reply
* Re: ax88796: add superh to kconfig depencencies
From: Paul Mundt @ 2007-10-09 13:47 UTC (permalink / raw)
To: Magnus Damm; +Cc: netdev, linuxsh-dev
In-Reply-To: <20071009125138.28915.30999.sendpatchset@clockwork.opensource.se>
On Tue, Oct 09, 2007 at 09:51:38PM +0900, Magnus Damm wrote:
> ax88796: add superh to kconfig depencencies
>
> This patch adds sh architecture support to the ax88796 kconfig.
>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
Acked-by: Paul Mundt <lethal@linux-sh.org>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
^ permalink raw reply
* [PATCH] ipv4: kernel panic when only one unsecured port available
From: Anton Arapov @ 2007-10-09 14:01 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]
Steps to reproduce:
Server:
[root@server ~]# cat /etc/exports
/export *(ro,insecure)
// there is insecure ... I am using ports like "1024 to 61000"
[root@server ~] service nfs restart
Client:
1.[root@client ~]# echo 32768 32768 > /proc/sys/net/ipv4/ip_local_port_range
32768 32768
// two same numbers, for ex "32769 32769" etc.
2.[root@client ~]# cat /proc/sys/net/ipv4/ip_local_port_range
32768 32768
3.[root@client ~]# mount server:/export /import
Actual results:
Kernel always panics
--------------------------------------------------------------------
[PATCH] ipv4: kernel panic when only one unsecured port available
Patch prevents division by zero. Kernel panics if only one
unsecured port available.
Signed-off-by: Anton Arapov <aarapov@redhat.com>
---
net/ipv4/inet_connection_sock.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index fbe7714..00ad079 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -80,7 +80,7 @@ int inet_csk_get_port(struct inet_hashinfo *hashinfo,
int low = sysctl_local_port_range[0];
int high = sysctl_local_port_range[1];
int remaining = (high - low) + 1;
- int rover = net_random() % (high - low) + low;
+ int rover = net_random() % remaining + low;
do {
head = &hashinfo->bhash[inet_bhashfn(rover, hashinfo->bhash_size)];
--
Anton Arapov, <aarapov@redhat.com>
Kernel Development, Red Hat
GPG Key ID: 0x6FA8C812
[-- Attachment #2: Type: application/pgp-signature, Size: 188 bytes --]
^ permalink raw reply related
* [0/7] IPsec: More work on async crypto on output
From: Herbert Xu @ 2007-10-09 14:35 UTC (permalink / raw)
To: David S. Miller, netdev
Hi Dave:
Here's another round of patches which ends with the moving down
of the state lock into x->type->output. I need to stop getting
distracted by fixing bugs and concentrate on creating them :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH 1/7] [IPSEC]: Remove bogus ref count in xfrm_secpath_reject
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[IPSEC]: Remove bogus ref count in xfrm_secpath_reject
Constructs of the form
xfrm_state_hold(x);
foo(x);
xfrm_state_put(x);
tend to be broken because foo is either synchronous where this is totally
unnecessary or if foo is asynchronous then the reference count is in the
wrong spot.
In the case of xfrm_secpath_reject, the function is synchronous and therefore
we should just kill the reference count.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/xfrm/xfrm_policy.c | 6 +-----
1 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 76f172f..af27c19 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1682,17 +1682,13 @@ static inline int
xfrm_secpath_reject(int idx, struct sk_buff *skb, struct flowi *fl)
{
struct xfrm_state *x;
- int err;
if (!skb->sp || idx < 0 || idx >= skb->sp->len)
return 0;
x = skb->sp->xvec[idx];
if (!x->type->reject)
return 0;
- xfrm_state_hold(x);
- err = x->type->reject(x, skb, fl);
- xfrm_state_put(x);
- return err;
+ return x->type->reject(x, skb, fl);
}
/* When skb is transformed back to its "native" form, we have to
^ permalink raw reply related
* [PATCH 2/7] [IPSEC]: Store IPv6 nh pointer in mac_header on output
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[IPSEC]: Store IPv6 nh pointer in mac_header on output
Current the x->mode->output functions store the IPv6 nh pointer in the
skb network header. This is inconvenient because the network header then
has to be fixed up before the packet can leave the IPsec stack. The mac
header field is unused on output so we can use that to store this instead.
This patch does that and removes the network header fix-up in xfrm_output.
It also uses ipv6_hdr where appropriate in the x->type->output functions.
There is also a minor clean-up in esp4 to make it use the same code as
esp6 to help any subsequent effort to merge the two.
Lastly it kills two redundant skb_set_* statements in BEET that were
simply copied over from transport mode.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/ipv4/esp4.c | 2 +-
net/ipv6/ah6.c | 6 +++---
net/ipv6/esp6.c | 7 ++++---
net/ipv6/ipcomp6.c | 6 +++---
net/ipv6/mip6.c | 12 ++++++------
net/ipv6/xfrm6_mode_beet.c | 16 +++++++---------
net/ipv6/xfrm6_mode_ro.c | 13 +++++++------
net/ipv6/xfrm6_mode_transport.c | 13 +++++++------
net/ipv6/xfrm6_mode_tunnel.c | 13 +++++++------
net/ipv6/xfrm6_tunnel.c | 2 +-
net/xfrm/xfrm_output.c | 2 --
11 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 0f62af9..ffd5653 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -59,7 +59,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
tail[clen - skb->len - 2] = (clen - skb->len) - 2;
pskb_put(skb, trailer, clen - skb->len);
- __skb_push(skb, skb->data - skb_network_header(skb));
+ __skb_push(skb, -skb_network_offset(skb));
top_iph = ip_hdr(skb);
esph = (struct ip_esp_hdr *)(skb_network_header(skb) +
top_iph->ihl * 4);
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index ae68a90..ff904a7 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -235,11 +235,11 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
char hdrs[0];
} *tmp_ext;
- top_iph = (struct ipv6hdr *)skb->data;
+ top_iph = ipv6_hdr(skb);
top_iph->payload_len = htons(skb->len - sizeof(*top_iph));
- nexthdr = *skb_network_header(skb);
- *skb_network_header(skb) = IPPROTO_AH;
+ nexthdr = *skb_mac_header(skb);
+ *skb_mac_header(skb) = IPPROTO_AH;
/* When there are no extension headers, we only need to save the first
* 8 bytes of the base IP header.
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 0c5fb81..9fc1940 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -88,11 +88,12 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
tail[clen-skb->len - 2] = (clen - skb->len) - 2;
pskb_put(skb, trailer, clen - skb->len);
- top_iph = (struct ipv6hdr *)__skb_push(skb, hdr_len);
+ __skb_push(skb, -skb_network_offset(skb));
+ top_iph = ipv6_hdr(skb);
esph = (struct ipv6_esp_hdr *)skb_transport_header(skb);
top_iph->payload_len = htons(skb->len + alen - sizeof(*top_iph));
- *(skb_tail_pointer(trailer) - 1) = *skb_network_header(skb);
- *skb_network_header(skb) = IPPROTO_ESP;
+ *(skb_tail_pointer(trailer) - 1) = *skb_mac_header(skb);
+ *skb_mac_header(skb) = IPPROTO_ESP;
esph->spi = x->id.spi;
esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 91b2a75..71a14c0 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -157,15 +157,15 @@ static int ipcomp6_output(struct xfrm_state *x, struct sk_buff *skb)
pskb_trim(skb, hdr_len + dlen + sizeof(struct ip_comp_hdr));
/* insert ipcomp header and replace datagram */
- top_iph = (struct ipv6hdr *)skb->data;
+ top_iph = ipv6_hdr(skb);
top_iph->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
ipch = (struct ipv6_comp_hdr *)start;
- ipch->nexthdr = *skb_network_header(skb);
+ ipch->nexthdr = *skb_mac_header(skb);
ipch->flags = 0;
ipch->cpi = htons((u16 )ntohl(x->id.spi));
- *skb_network_header(skb) = IPPROTO_COMP;
+ *skb_mac_header(skb) = IPPROTO_COMP;
out_ok:
return 0;
diff --git a/net/ipv6/mip6.c b/net/ipv6/mip6.c
index 8a1399c..7261c29 100644
--- a/net/ipv6/mip6.c
+++ b/net/ipv6/mip6.c
@@ -153,11 +153,11 @@ static int mip6_destopt_output(struct xfrm_state *x, struct sk_buff *skb)
u8 nexthdr;
int len;
- iph = (struct ipv6hdr *)skb->data;
+ iph = ipv6_hdr(skb);
iph->payload_len = htons(skb->len - sizeof(*iph));
- nexthdr = *skb_network_header(skb);
- *skb_network_header(skb) = IPPROTO_DSTOPTS;
+ nexthdr = *skb_mac_header(skb);
+ *skb_mac_header(skb) = IPPROTO_DSTOPTS;
dstopt = (struct ipv6_destopt_hdr *)skb_transport_header(skb);
dstopt->nexthdr = nexthdr;
@@ -365,11 +365,11 @@ static int mip6_rthdr_output(struct xfrm_state *x, struct sk_buff *skb)
struct rt2_hdr *rt2;
u8 nexthdr;
- iph = (struct ipv6hdr *)skb->data;
+ iph = ipv6_hdr(skb);
iph->payload_len = htons(skb->len - sizeof(*iph));
- nexthdr = *skb_network_header(skb);
- *skb_network_header(skb) = IPPROTO_ROUTING;
+ nexthdr = *skb_mac_header(skb);
+ *skb_mac_header(skb) = IPPROTO_ROUTING;
rt2 = (struct rt2_hdr *)skb_transport_header(skb);
rt2->rt_hdr.nexthdr = nexthdr;
diff --git a/net/ipv6/xfrm6_mode_beet.c b/net/ipv6/xfrm6_mode_beet.c
index 2e61d6d..65e6b2a 100644
--- a/net/ipv6/xfrm6_mode_beet.c
+++ b/net/ipv6/xfrm6_mode_beet.c
@@ -26,10 +26,11 @@
* payload_len
*
* On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header. The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
*/
static int xfrm6_beet_output(struct xfrm_state *x, struct sk_buff *skb)
{
@@ -41,15 +42,12 @@ static int xfrm6_beet_output(struct xfrm_state *x, struct sk_buff *skb)
iph = ipv6_hdr(skb);
hdr_len = ip6_find_1stfragopt(skb, &prevhdr);
- skb_set_network_header(skb,
- (prevhdr - x->props.header_len) - skb->data);
- skb_set_transport_header(skb, hdr_len);
memmove(skb->data, iph, hdr_len);
+ skb_set_mac_header(skb, offsetof(struct ipv6hdr, nexthdr));
skb_reset_network_header(skb);
+ skb_set_transport_header(skb, sizeof(struct ipv6hdr));
top_iph = ipv6_hdr(skb);
- skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
- skb->network_header += offsetof(struct ipv6hdr, nexthdr);
ipv6_addr_copy(&top_iph->saddr, (struct in6_addr *)&x->props.saddr);
ipv6_addr_copy(&top_iph->daddr, (struct in6_addr *)&x->id.daddr);
diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
index a156373..2575804 100644
--- a/net/ipv6/xfrm6_mode_ro.c
+++ b/net/ipv6/xfrm6_mode_ro.c
@@ -39,10 +39,11 @@
* space for the route optimization header.
*
* On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header. The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
*/
static int xfrm6_ro_output(struct xfrm_state *x, struct sk_buff *skb)
{
@@ -54,8 +55,8 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct sk_buff *skb)
iph = ipv6_hdr(skb);
hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
- skb_set_network_header(skb,
- (prevhdr - x->props.header_len) - skb->data);
+ skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
+ skb_reset_network_header(skb);
skb_set_transport_header(skb, hdr_len);
memmove(skb->data, iph, hdr_len);
diff --git a/net/ipv6/xfrm6_mode_transport.c b/net/ipv6/xfrm6_mode_transport.c
index c026bfe..65c166b 100644
--- a/net/ipv6/xfrm6_mode_transport.c
+++ b/net/ipv6/xfrm6_mode_transport.c
@@ -20,10 +20,11 @@
* space for the encapsulation header.
*
* On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header. The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
*/
static int xfrm6_transport_output(struct xfrm_state *x, struct sk_buff *skb)
{
@@ -35,8 +36,8 @@ static int xfrm6_transport_output(struct xfrm_state *x, struct sk_buff *skb)
iph = ipv6_hdr(skb);
hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
- skb_set_network_header(skb,
- (prevhdr - x->props.header_len) - skb->data);
+ skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
+ skb_reset_network_header(skb);
skb_set_transport_header(skb, hdr_len);
memmove(skb->data, iph, hdr_len);
return 0;
diff --git a/net/ipv6/xfrm6_mode_tunnel.c b/net/ipv6/xfrm6_mode_tunnel.c
index 9fc95bc..3dd40af 100644
--- a/net/ipv6/xfrm6_mode_tunnel.c
+++ b/net/ipv6/xfrm6_mode_tunnel.c
@@ -38,10 +38,11 @@ static inline void ip6ip_ecn_decapsulate(struct sk_buff *skb)
* payload_len
*
* On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header. The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
*/
static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
{
@@ -53,10 +54,10 @@ static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
skb_push(skb, x->props.header_len);
iph = ipv6_hdr(skb);
+ skb_set_mac_header(skb, offsetof(struct ipv6hdr, nexthdr));
skb_reset_network_header(skb);
+ skb_set_transport_header(skb, sizeof(struct ipv6hdr));
top_iph = ipv6_hdr(skb);
- skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
- skb->network_header += offsetof(struct ipv6hdr, nexthdr);
top_iph->version = 6;
if (xdst->route->ops->family == AF_INET6) {
diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c
index 30f3236..aeb0607 100644
--- a/net/ipv6/xfrm6_tunnel.c
+++ b/net/ipv6/xfrm6_tunnel.c
@@ -244,7 +244,7 @@ static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
{
struct ipv6hdr *top_iph;
- top_iph = (struct ipv6hdr *)skb->data;
+ top_iph = ipv6_hdr(skb);
top_iph->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
return 0;
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 8c85211..9847bae 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -76,8 +76,6 @@ int xfrm_output(struct sk_buff *skb)
spin_unlock_bh(&x->lock);
- skb_reset_network_header(skb);
-
if (!(skb->dst = dst_pop(dst))) {
err = -EHOSTUNREACH;
goto error_nolock;
^ permalink raw reply related
* [PATCH 3/7] [IPSEC]: Remove gratuitous km wake-up events on ACQUIRE
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[IPSEC]: Remove gratuitous km wake-up events on ACQUIRE
There is no point in waking people up when creating/updating larval states
because they'll just go back to sleep again as larval states by definition
cannot be found by xfrm_state_find.
We should only wake them up when the larvals mature or die.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/xfrm/xfrm_state.c | 2 --
1 files changed, 2 deletions(-)
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a00745a..0d07f6b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -848,7 +848,6 @@ static struct xfrm_state *__find_acq_core(unsigned short family, u8 mode, u32 re
hlist_add_head(&x->bydst, xfrm_state_bydst+h);
h = xfrm_src_hash(daddr, saddr, family);
hlist_add_head(&x->bysrc, xfrm_state_bysrc+h);
- wake_up(&km_waitq);
xfrm_state_num++;
@@ -1311,7 +1310,6 @@ xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
h = xfrm_spi_hash(&x->id.daddr, x->id.spi, x->id.proto, x->props.family);
hlist_add_head(&x->byspi, xfrm_state_byspi+h);
spin_unlock_bh(&xfrm_state_lock);
- wake_up(&km_waitq);
}
}
EXPORT_SYMBOL(xfrm_alloc_spi);
^ permalink raw reply related
* [PATCH 4/7] [IPSEC]: Move common code into xfrm_alloc_spi
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[IPSEC]: Move common code into xfrm_alloc_spi
This patch moves some common code that conceptually belongs to the xfrm core
from af_key/xfrm_user into xfrm_alloc_spi.
In particular, the spin lock on the state is now taken inside xfrm_alloc_spi.
Previously it also protected the construction of the response PF_KEY/XFRM
messages to user-space. This is inconsistent as other identical constructions
are not protected by the state lock. This is bad because they in fact should
be protected but only in certain spots (so as not to hold the lock for too
long which may cause packet drops).
The SPI byte order conversion has also been moved.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
include/net/xfrm.h | 2 +-
net/key/af_key.c | 29 ++++++++++++-----------------
net/xfrm/xfrm_state.c | 26 ++++++++++++++++++++------
net/xfrm/xfrm_user.c | 13 ++++---------
4 files changed, 37 insertions(+), 33 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 064a4ca..1c116dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1084,7 +1084,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(u8 type, int dir,
struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
u32 xfrm_get_acqseq(void);
-void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
+extern int xfrm_alloc_spi(struct xfrm_state *x, u32 minspi, u32 maxspi);
struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
xfrm_address_t *daddr, xfrm_address_t *saddr,
int create, unsigned short family);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index ff5c3d0..143d46f 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1253,8 +1253,11 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
struct sadb_x_sa2 *sa2;
struct sadb_address *saddr, *daddr;
struct sadb_msg *out_hdr;
+ struct sadb_spirange *range;
struct xfrm_state *x = NULL;
int mode;
+ int err;
+ u32 min_spi, max_spi;
u32 reqid;
u8 proto;
unsigned short family;
@@ -1309,25 +1312,17 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
if (x == NULL)
return -ENOENT;
- resp_skb = ERR_PTR(-ENOENT);
-
- spin_lock_bh(&x->lock);
- if (x->km.state != XFRM_STATE_DEAD) {
- struct sadb_spirange *range = ext_hdrs[SADB_EXT_SPIRANGE-1];
- u32 min_spi, max_spi;
+ min_spi = 0x100;
+ max_spi = 0x0fffffff;
- if (range != NULL) {
- min_spi = range->sadb_spirange_min;
- max_spi = range->sadb_spirange_max;
- } else {
- min_spi = 0x100;
- max_spi = 0x0fffffff;
- }
- xfrm_alloc_spi(x, htonl(min_spi), htonl(max_spi));
- if (x->id.spi)
- resp_skb = pfkey_xfrm_state2msg(x, 0, 3);
+ range = ext_hdrs[SADB_EXT_SPIRANGE-1];
+ if (range) {
+ min_spi = range->sadb_spirange_min;
+ max_spi = range->sadb_spirange_max;
}
- spin_unlock_bh(&x->lock);
+
+ err = xfrm_alloc_spi(x, min_spi, max_spi);
+ resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x, 0, 3);
if (IS_ERR(resp_skb)) {
xfrm_state_put(x);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0d07f6b..344f0a6 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1275,26 +1275,33 @@ u32 xfrm_get_acqseq(void)
}
EXPORT_SYMBOL(xfrm_get_acqseq);
-void
-xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
+int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
{
unsigned int h;
struct xfrm_state *x0;
+ int err = -ENOENT;
+ __be32 minspi = htonl(low);
+ __be32 maxspi = htonl(high);
+
+ spin_lock_bh(&x->lock);
+ if (x->km.state == XFRM_STATE_DEAD)
+ goto unlock;
+ err = 0;
if (x->id.spi)
- return;
+ goto unlock;
+
+ err = -ENOENT;
if (minspi == maxspi) {
x0 = xfrm_state_lookup(&x->id.daddr, minspi, x->id.proto, x->props.family);
if (x0) {
xfrm_state_put(x0);
- return;
+ goto unlock;
}
x->id.spi = minspi;
} else {
u32 spi = 0;
- u32 low = ntohl(minspi);
- u32 high = ntohl(maxspi);
for (h=0; h<high-low+1; h++) {
spi = low + net_random()%(high-low+1);
x0 = xfrm_state_lookup(&x->id.daddr, htonl(spi), x->id.proto, x->props.family);
@@ -1310,7 +1317,14 @@ xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
h = xfrm_spi_hash(&x->id.daddr, x->id.spi, x->id.proto, x->props.family);
hlist_add_head(&x->byspi, xfrm_state_byspi+h);
spin_unlock_bh(&xfrm_state_lock);
+
+ err = 0;
}
+
+unlock:
+ spin_unlock_bh(&x->lock);
+
+ return err;
}
EXPORT_SYMBOL(xfrm_alloc_spi);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8e10e90..52c7fce 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -784,16 +784,11 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
if (x == NULL)
goto out_noput;
- resp_skb = ERR_PTR(-ENOENT);
-
- spin_lock_bh(&x->lock);
- if (x->km.state != XFRM_STATE_DEAD) {
- xfrm_alloc_spi(x, htonl(p->min), htonl(p->max));
- if (x->id.spi)
- resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq);
- }
- spin_unlock_bh(&x->lock);
+ err = xfrm_alloc_spi(x, p->min, p->max);
+ if (err)
+ goto out;
+ resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq);
if (IS_ERR(resp_skb)) {
err = PTR_ERR(resp_skb);
goto out;
^ permalink raw reply related
* [PATCH 5/7] [XFRM] user: Move attribute copying code into copy_to_user_state_extra
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[XFRM] user: Move attribute copying code into copy_to_user_state_extra
Here's a good example of code duplication leading to code rot. The
notification patch did its own netlink message creation for xfrm states.
It duplicated code that was already in dump_one_state. Guess what, the
next time (and the time after) when someone updated dump_one_state the
notification path got zilch.
This patch moves that code from dump_one_state to copy_to_user_state_extra
and uses it in xfrm_notify_sa too. Unfortunately whoever updates this
still needs to update xfrm_sa_len since the notification path wants to
know the exact size for allocation.
At least I've added a comment saying so and if someone still forgest, we'll
have a WARN_ON telling us so.
I also changed the security size calculation to use xfrm_user_sec_ctx since
that's what we actually put into the skb. However it makes no practical
difference since it has the same size as xfrm_sec_ctx.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/xfrm/xfrm_user.c | 76 +++++++++++++++++++++++++++++++--------------------
1 files changed, 47 insertions(+), 29 deletions(-)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 52c7fce..2cbbe5e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -483,9 +483,9 @@ struct xfrm_dump_info {
static int copy_sec_ctx(struct xfrm_sec_ctx *s, struct sk_buff *skb)
{
- int ctx_size = sizeof(struct xfrm_sec_ctx) + s->ctx_len;
struct xfrm_user_sec_ctx *uctx;
struct nlattr *attr;
+ int ctx_size = sizeof(*uctx) + s->ctx_len;
attr = nla_reserve(skb, XFRMA_SEC_CTX, ctx_size);
if (attr == NULL)
@@ -502,23 +502,11 @@ static int copy_sec_ctx(struct xfrm_sec_ctx *s, struct sk_buff *skb)
return 0;
}
-static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+/* Don't change this without updating xfrm_sa_len! */
+static int copy_to_user_state_extra(struct xfrm_state *x,
+ struct xfrm_usersa_info *p,
+ struct sk_buff *skb)
{
- struct xfrm_dump_info *sp = ptr;
- struct sk_buff *in_skb = sp->in_skb;
- struct sk_buff *skb = sp->out_skb;
- struct xfrm_usersa_info *p;
- struct nlmsghdr *nlh;
-
- if (sp->this_idx < sp->start_idx)
- goto out;
-
- nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
- XFRM_MSG_NEWSA, sizeof(*p), sp->nlmsg_flags);
- if (nlh == NULL)
- return -EMSGSIZE;
-
- p = nlmsg_data(nlh);
copy_to_user_state(x, p);
if (x->aalg)
@@ -540,6 +528,35 @@ static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
if (x->lastused)
NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
+ return 0;
+
+nla_put_failure:
+ return -EMSGSIZE;
+}
+
+static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+{
+ struct xfrm_dump_info *sp = ptr;
+ struct sk_buff *in_skb = sp->in_skb;
+ struct sk_buff *skb = sp->out_skb;
+ struct xfrm_usersa_info *p;
+ struct nlmsghdr *nlh;
+ int err;
+
+ if (sp->this_idx < sp->start_idx)
+ goto out;
+
+ nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
+ XFRM_MSG_NEWSA, sizeof(*p), sp->nlmsg_flags);
+ if (nlh == NULL)
+ return -EMSGSIZE;
+
+ p = nlmsg_data(nlh);
+
+ err = copy_to_user_state_extra(x, p, skb);
+ if (err)
+ goto nla_put_failure;
+
nlmsg_end(skb, nlh);
out:
sp->this_idx++;
@@ -547,7 +564,7 @@ out:
nla_put_failure:
nlmsg_cancel(skb, nlh);
- return -EMSGSIZE;
+ return err;
}
static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)
@@ -1973,6 +1990,14 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
l += nla_total_size(sizeof(*x->calg));
if (x->encap)
l += nla_total_size(sizeof(*x->encap));
+ if (x->security)
+ l += nla_total_size(sizeof(struct xfrm_user_sec_ctx) +
+ x->security->ctx_len);
+ if (x->coaddr)
+ l += nla_total_size(sizeof(*x->coaddr));
+
+ /* Must count this as this may become non-zero behind our back. */
+ l += nla_total_size(sizeof(x->lastused));
return l;
}
@@ -2018,23 +2043,16 @@ static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
p = nla_data(attr);
}
- copy_to_user_state(x, p);
-
- if (x->aalg)
- NLA_PUT(skb, XFRMA_ALG_AUTH, alg_len(x->aalg), x->aalg);
- if (x->ealg)
- NLA_PUT(skb, XFRMA_ALG_CRYPT, alg_len(x->ealg), x->ealg);
- if (x->calg)
- NLA_PUT(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
-
- if (x->encap)
- NLA_PUT(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+ if (copy_to_user_state_extra(x, p, skb))
+ goto nla_put_failure;
nlmsg_end(skb, nlh);
return nlmsg_multicast(xfrm_nl, skb, 0, XFRMNLGRP_SA, GFP_ATOMIC);
nla_put_failure:
+ /* Somebody screwed up with xfrm_sa_len! */
+ WARN_ON(1);
kfree_skb(skb);
return -1;
}
^ permalink raw reply related
* [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space
From: Herbert Xu @ 2007-10-09 14:36 UTC (permalink / raw)
To: David S. Miller, netdev, Herbert Xu
In-Reply-To: <20071009143517.GA20486@gondor.apana.org.au>
[IPSEC]: Lock state when copying non-atomic fields to user-space
This patch adds locking so that when we're copying non-atomic fields such as
life-time or coaddr to user-space we don't get a partial result.
For af_key I've changed every instance of pfkey_xfrm_state2msg apart from
expiration notification to include the keys and life-times. This is in-line
with XFRM behaviour.
The actual cases affected are:
* pfkey_getspi: No change as we don't have any keys to copy.
* key_notify_sa:
+ ADD/UPD: This wouldn't work otherwise.
+ DEL: It can't hurt.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/key/af_key.c | 35 +++++++++++++++++++++++++----------
net/xfrm/xfrm_user.c | 14 ++++++++------
2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 143d46f..7969f8a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -655,7 +655,8 @@ static inline int pfkey_mode_to_xfrm(int mode)
}
}
-static struct sk_buff * pfkey_xfrm_state2msg(struct xfrm_state *x, int add_keys, int hsc)
+static struct sk_buff *__pfkey_xfrm_state2msg(struct xfrm_state *x,
+ int add_keys, int hsc)
{
struct sk_buff *skb;
struct sadb_msg *hdr;
@@ -1009,6 +1010,24 @@ static struct sk_buff * pfkey_xfrm_state2msg(struct xfrm_state *x, int add_keys,
return skb;
}
+
+static inline struct sk_buff *pfkey_xfrm_state2msg(struct xfrm_state *x)
+{
+ struct sk_buff *skb;
+
+ spin_lock_bh(&x->lock);
+ skb = __pfkey_xfrm_state2msg(x, 1, 3);
+ spin_unlock_bh(&x->lock);
+
+ return skb;
+}
+
+static inline struct sk_buff *pfkey_xfrm_state2msg_expire(struct xfrm_state *x,
+ int hsc)
+{
+ return __pfkey_xfrm_state2msg(x, 0, hsc);
+}
+
static struct xfrm_state * pfkey_msg2xfrm_state(struct sadb_msg *hdr,
void **ext_hdrs)
{
@@ -1322,7 +1341,7 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
}
err = xfrm_alloc_spi(x, min_spi, max_spi);
- resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x, 0, 3);
+ resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x);
if (IS_ERR(resp_skb)) {
xfrm_state_put(x);
@@ -1412,12 +1431,8 @@ static int key_notify_sa(struct xfrm_state *x, struct km_event *c)
{
struct sk_buff *skb;
struct sadb_msg *hdr;
- int hsc = 3;
-
- if (c->event == XFRM_MSG_DELSA)
- hsc = 0;
- skb = pfkey_xfrm_state2msg(x, 0, hsc);
+ skb = pfkey_xfrm_state2msg(x);
if (IS_ERR(skb))
return PTR_ERR(skb);
@@ -1529,7 +1544,7 @@ static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr,
if (x == NULL)
return -ESRCH;
- out_skb = pfkey_xfrm_state2msg(x, 1, 3);
+ out_skb = pfkey_xfrm_state2msg(x);
proto = x->id.proto;
xfrm_state_put(x);
if (IS_ERR(out_skb))
@@ -1709,7 +1724,7 @@ static int dump_sa(struct xfrm_state *x, int count, void *ptr)
struct sk_buff *out_skb;
struct sadb_msg *out_hdr;
- out_skb = pfkey_xfrm_state2msg(x, 1, 3);
+ out_skb = pfkey_xfrm_state2msg(x);
if (IS_ERR(out_skb))
return PTR_ERR(out_skb);
@@ -2910,7 +2925,7 @@ static int key_notify_sa_expire(struct xfrm_state *x, struct km_event *c)
else
hsc = 1;
- out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
+ out_skb = pfkey_xfrm_state2msg_expire(x, hsc);
if (IS_ERR(out_skb))
return PTR_ERR(out_skb);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 2cbbe5e..5238f6a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -507,8 +507,16 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
struct xfrm_usersa_info *p,
struct sk_buff *skb)
{
+ spin_lock_bh(&x->lock);
copy_to_user_state(x, p);
+ if (x->coaddr)
+ NLA_PUT(skb, XFRMA_COADDR, sizeof(*x->coaddr), x->coaddr);
+
+ if (x->lastused)
+ NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
+ spin_unlock_bh(&x->lock);
+
if (x->aalg)
NLA_PUT(skb, XFRMA_ALG_AUTH, alg_len(x->aalg), x->aalg);
if (x->ealg)
@@ -522,12 +530,6 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
if (x->security && copy_sec_ctx(x->security, skb) < 0)
goto nla_put_failure;
- if (x->coaddr)
- NLA_PUT(skb, XFRMA_COADDR, sizeof(*x->coaddr), x->coaddr);
-
- if (x->lastused)
- NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
-
return 0;
nla_put_failure:
^ permalink raw reply related
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