* netfilter -stable 00/02: netfilter -stable fixes
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
The following two patches fix two bug in netfilter:
- a bug in TCP conntrack sequence tracking when used with NAT helpers that
enlarge packets
- a regression in the xt_connlimit match introduced in 2.6.29, causing
false negatives
Please apply, thanks.
include/net/netfilter/nf_conntrack.h | 8 +--
include/net/netfilter/nf_nat_helper.h | 4 ++
net/ipv4/netfilter/nf_nat_core.c | 3 +
net/ipv4/netfilter/nf_nat_helper.c | 34 +++++++++++-----
net/netfilter/nf_conntrack_core.c | 8 ++++
net/netfilter/nf_conntrack_proto_tcp.c | 64 +++++++++++++-------------------
net/netfilter/xt_connlimit.c | 10 ++---
7 files changed, 71 insertions(+), 60 deletions(-)
Jan Engelhardt (1):
netfilter: xt_connlimit: fix regression caused by zero family value
Jozsef Kadlecsik (1):
netfilter: nf_nat: fix NAT issue in 2.6.30.4+
^ permalink raw reply
* netfilter -stable 01/02: nf_nat: fix NAT issue in 2.6.30.4+
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
In-Reply-To: <20091110104014.8250.89589.sendpatchset@x2.localnet>
commit eb3336f5b440ae5c15d09abe20d7bdd45a88d157
Author: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Tue Nov 10 10:53:08 2009 +0100
netfilter: nf_nat: fix NAT issue in 2.6.30.4+
Upstream commit f9dd09c7:
Vitezslav Samel discovered that since 2.6.30.4+ active FTP can not work
over NAT. The "cause" of the problem was a fix of unacknowledged data
detection with NAT (commit a3a9f79e361e864f0e9d75ebe2a0cb43d17c4272).
However, actually, that fix uncovered a long standing bug in TCP conntrack:
when NAT was enabled, we simply updated the max of the right edge of
the segments we have seen (td_end), by the offset NAT produced with
changing IP/port in the data. However, we did not update the other parameter
(td_maxend) which is affected by the NAT offset. Thus that could drift
away from the correct value and thus resulted breaking active FTP.
The patch below fixes the issue by *not* updating the conntrack parameters
from NAT, but instead taking into account the NAT offsets in conntrack in a
consistent way. (Updating from NAT would be more harder and expensive because
it'd need to re-calculate parameters we already calculated in conntrack.)
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 5d9a848..a96b835 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -252,11 +252,9 @@ static inline bool nf_ct_kill(struct nf_conn *ct)
}
/* These are for NAT. Icky. */
-/* Update TCP window tracking data when NAT mangles the packet */
-extern void nf_conntrack_tcp_update(const struct sk_buff *skb,
- unsigned int dataoff,
- struct nf_conn *ct, int dir,
- s16 offset);
+extern s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
/* Fake conntrack entry for untracked connections */
extern struct nf_conn nf_conntrack_untracked;
diff --git a/include/net/netfilter/nf_nat_helper.h b/include/net/netfilter/nf_nat_helper.h
index 237a961..4222220 100644
--- a/include/net/netfilter/nf_nat_helper.h
+++ b/include/net/netfilter/nf_nat_helper.h
@@ -32,4 +32,8 @@ extern int (*nf_nat_seq_adjust_hook)(struct sk_buff *skb,
* to port ct->master->saved_proto. */
extern void nf_nat_follow_master(struct nf_conn *ct,
struct nf_conntrack_expect *this);
+
+extern s16 nf_nat_get_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
#endif
diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index b6ddd56..d396abf 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -750,6 +750,8 @@ static int __init nf_nat_init(void)
BUG_ON(nfnetlink_parse_nat_setup_hook != NULL);
rcu_assign_pointer(nfnetlink_parse_nat_setup_hook,
nfnetlink_parse_nat_setup);
+ BUG_ON(nf_ct_nat_offset != NULL);
+ rcu_assign_pointer(nf_ct_nat_offset, nf_nat_get_offset);
return 0;
cleanup_extend:
@@ -764,6 +766,7 @@ static void __exit nf_nat_cleanup(void)
nf_ct_extend_unregister(&nat_extend);
rcu_assign_pointer(nf_nat_seq_adjust_hook, NULL);
rcu_assign_pointer(nfnetlink_parse_nat_setup_hook, NULL);
+ rcu_assign_pointer(nf_ct_nat_offset, NULL);
synchronize_net();
}
diff --git a/net/ipv4/netfilter/nf_nat_helper.c b/net/ipv4/netfilter/nf_nat_helper.c
index 05ede41..79e28ad 100644
--- a/net/ipv4/netfilter/nf_nat_helper.c
+++ b/net/ipv4/netfilter/nf_nat_helper.c
@@ -73,6 +73,28 @@ adjust_tcp_sequence(u32 seq,
DUMP_OFFSET(this_way);
}
+/* Get the offset value, for conntrack */
+s16 nf_nat_get_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq)
+{
+ struct nf_conn_nat *nat = nfct_nat(ct);
+ struct nf_nat_seq *this_way;
+ s16 offset;
+
+ if (!nat)
+ return 0;
+
+ this_way = &nat->seq[dir];
+ spin_lock_bh(&nf_nat_seqofs_lock);
+ offset = after(seq, this_way->correction_pos)
+ ? this_way->offset_after : this_way->offset_before;
+ spin_unlock_bh(&nf_nat_seqofs_lock);
+
+ return offset;
+}
+EXPORT_SYMBOL_GPL(nf_nat_get_offset);
+
/* Frobs data inside this packet, which is linear. */
static void mangle_contents(struct sk_buff *skb,
unsigned int dataoff,
@@ -189,11 +211,6 @@ nf_nat_mangle_tcp_packet(struct sk_buff *skb,
adjust_tcp_sequence(ntohl(tcph->seq),
(int)rep_len - (int)match_len,
ct, ctinfo);
- /* Tell TCP window tracking about seq change */
- nf_conntrack_tcp_update(skb, ip_hdrlen(skb),
- ct, CTINFO2DIR(ctinfo),
- (int)rep_len - (int)match_len);
-
nf_conntrack_event_cache(IPCT_NATSEQADJ, ct);
}
return 1;
@@ -415,12 +432,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
tcph->seq = newseq;
tcph->ack_seq = newack;
- if (!nf_nat_sack_adjust(skb, tcph, ct, ctinfo))
- return 0;
-
- nf_conntrack_tcp_update(skb, ip_hdrlen(skb), ct, dir, seqoff);
-
- return 1;
+ return nf_nat_sack_adjust(skb, tcph, ct, ctinfo);
}
/* Setup NAT on this expected conntrack so it follows master. */
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 0d961ee..56ed268 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1296,6 +1296,11 @@ err_stat:
return ret;
}
+s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
+EXPORT_SYMBOL_GPL(nf_ct_nat_offset);
+
int nf_conntrack_init(struct net *net)
{
int ret;
@@ -1313,6 +1318,9 @@ int nf_conntrack_init(struct net *net)
/* For use by REJECT target */
rcu_assign_pointer(ip_ct_attach, nf_conntrack_attach);
rcu_assign_pointer(nf_ct_destroy, destroy_conntrack);
+
+ /* Howto get NAT offsets */
+ rcu_assign_pointer(nf_ct_nat_offset, NULL);
}
return 0;
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index a38bc22..718c334 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -482,6 +482,21 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
}
}
+#ifdef CONFIG_NF_NAT_NEEDED
+static inline s16 nat_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq)
+{
+ typeof(nf_ct_nat_offset) get_offset = rcu_dereference(nf_ct_nat_offset);
+
+ return get_offset != NULL ? get_offset(ct, dir, seq) : 0;
+}
+#define NAT_OFFSET(pf, ct, dir, seq) \
+ (pf == NFPROTO_IPV4 ? nat_offset(ct, dir, seq) : 0)
+#else
+#define NAT_OFFSET(pf, ct, dir, seq) 0
+#endif
+
static bool tcp_in_window(const struct nf_conn *ct,
struct ip_ct_tcp *state,
enum ip_conntrack_dir dir,
@@ -496,6 +511,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
struct ip_ct_tcp_state *receiver = &state->seen[!dir];
const struct nf_conntrack_tuple *tuple = &ct->tuplehash[dir].tuple;
__u32 seq, ack, sack, end, win, swin;
+ s16 receiver_offset;
bool res;
/*
@@ -509,11 +525,16 @@ static bool tcp_in_window(const struct nf_conn *ct,
if (receiver->flags & IP_CT_TCP_FLAG_SACK_PERM)
tcp_sack(skb, dataoff, tcph, &sack);
+ /* Take into account NAT sequence number mangling */
+ receiver_offset = NAT_OFFSET(pf, ct, !dir, ack - 1);
+ ack -= receiver_offset;
+ sack -= receiver_offset;
+
pr_debug("tcp_in_window: START\n");
pr_debug("tcp_in_window: ");
nf_ct_dump_tuple(tuple);
- pr_debug("seq=%u ack=%u sack=%u win=%u end=%u\n",
- seq, ack, sack, win, end);
+ pr_debug("seq=%u ack=%u+(%d) sack=%u+(%d) win=%u end=%u\n",
+ seq, ack, receiver_offset, sack, receiver_offset, win, end);
pr_debug("tcp_in_window: sender end=%u maxend=%u maxwin=%u scale=%i "
"receiver end=%u maxend=%u maxwin=%u scale=%i\n",
sender->td_end, sender->td_maxend, sender->td_maxwin,
@@ -599,8 +620,8 @@ static bool tcp_in_window(const struct nf_conn *ct,
pr_debug("tcp_in_window: ");
nf_ct_dump_tuple(tuple);
- pr_debug("seq=%u ack=%u sack =%u win=%u end=%u\n",
- seq, ack, sack, win, end);
+ pr_debug("seq=%u ack=%u+(%d) sack=%u+(%d) win=%u end=%u\n",
+ seq, ack, receiver_offset, sack, receiver_offset, win, end);
pr_debug("tcp_in_window: sender end=%u maxend=%u maxwin=%u scale=%i "
"receiver end=%u maxend=%u maxwin=%u scale=%i\n",
sender->td_end, sender->td_maxend, sender->td_maxwin,
@@ -686,7 +707,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
before(seq, sender->td_maxend + 1) ?
after(end, sender->td_end - receiver->td_maxwin - 1) ?
before(sack, receiver->td_end + 1) ?
- after(ack, receiver->td_end - MAXACKWINDOW(sender)) ? "BUG"
+ after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1) ? "BUG"
: "ACK is under the lower bound (possible overly delayed ACK)"
: "ACK is over the upper bound (ACKed data not seen yet)"
: "SEQ is under the lower bound (already ACKed data retransmitted)"
@@ -701,39 +722,6 @@ static bool tcp_in_window(const struct nf_conn *ct,
return res;
}
-#ifdef CONFIG_NF_NAT_NEEDED
-/* Update sender->td_end after NAT successfully mangled the packet */
-/* Caller must linearize skb at tcp header. */
-void nf_conntrack_tcp_update(const struct sk_buff *skb,
- unsigned int dataoff,
- struct nf_conn *ct, int dir,
- s16 offset)
-{
- const struct tcphdr *tcph = (const void *)skb->data + dataoff;
- const struct ip_ct_tcp_state *sender = &ct->proto.tcp.seen[dir];
- const struct ip_ct_tcp_state *receiver = &ct->proto.tcp.seen[!dir];
- __u32 end;
-
- end = segment_seq_plus_len(ntohl(tcph->seq), skb->len, dataoff, tcph);
-
- write_lock_bh(&tcp_lock);
- /*
- * We have to worry for the ack in the reply packet only...
- */
- if (ct->proto.tcp.seen[dir].td_end + offset == end)
- ct->proto.tcp.seen[dir].td_end = end;
- ct->proto.tcp.last_end = end;
- write_unlock_bh(&tcp_lock);
- pr_debug("tcp_update: sender end=%u maxend=%u maxwin=%u scale=%i "
- "receiver end=%u maxend=%u maxwin=%u scale=%i\n",
- sender->td_end, sender->td_maxend, sender->td_maxwin,
- sender->td_scale,
- receiver->td_end, receiver->td_maxend, receiver->td_maxwin,
- receiver->td_scale);
-}
-EXPORT_SYMBOL_GPL(nf_conntrack_tcp_update);
-#endif
-
#define TH_FIN 0x01
#define TH_SYN 0x02
#define TH_RST 0x04
^ permalink raw reply related
* netfilter -stable 02/02: xt_connlimit: fix regression caused by zero family value
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
In-Reply-To: <20091110104014.8250.89589.sendpatchset@x2.localnet>
commit 4dcaa8fa124a6d8ab0f64dc1c289b13a724f8fc1
Author: Jan Engelhardt <jengelh@medozas.de>
Date: Tue Nov 10 10:53:33 2009 +0100
netfilter: xt_connlimit: fix regression caused by zero family value
Upstream commit 539054a8:
Commit v2.6.28-rc1~717^2~109^2~2 was slightly incomplete; not all
instances of par->match->family were changed to par->family.
References: http://bugzilla.netfilter.org/show_bug.cgi?id=610
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 6809809..38f03f7 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -103,7 +103,7 @@ static int count_them(struct xt_connlimit_data *data,
const struct nf_conntrack_tuple *tuple,
const union nf_inet_addr *addr,
const union nf_inet_addr *mask,
- const struct xt_match *match)
+ u_int8_t family)
{
const struct nf_conntrack_tuple_hash *found;
struct xt_connlimit_conn *conn;
@@ -113,8 +113,7 @@ static int count_them(struct xt_connlimit_data *data,
bool addit = true;
int matches = 0;
-
- if (match->family == NFPROTO_IPV6)
+ if (family == NFPROTO_IPV6)
hash = &data->iphash[connlimit_iphash6(addr, mask)];
else
hash = &data->iphash[connlimit_iphash(addr->ip & mask->ip)];
@@ -157,8 +156,7 @@ static int count_them(struct xt_connlimit_data *data,
continue;
}
- if (same_source_net(addr, mask, &conn->tuple.src.u3,
- match->family))
+ if (same_source_net(addr, mask, &conn->tuple.src.u3, family))
/* same source network -> be counted! */
++matches;
nf_ct_put(found_ct);
@@ -207,7 +205,7 @@ connlimit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
spin_lock_bh(&info->data->lock);
connections = count_them(info->data, tuple_ptr, &addr,
- &info->mask, par->match);
+ &info->mask, par->family);
spin_unlock_bh(&info->data->lock);
if (connections < 0) {
^ permalink raw reply related
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 10:48 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF9406C.8010603@trash.net>
On Tue, Nov 10, 2009 at 6:29 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> I'm not sure how this will help, ifb device transmission is
> still serialized by the ingress queue lock and the mirred lock.
>
But not in ifb_thread. For firewall the main routine is after
netif_rx_ni(). For server applications, the packets belong to the same
flow are always routed to the same CPU, and the applications can
benefit with better localization.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 10:55 UTC (permalink / raw)
To: Changli Gao; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100248mbc14287p26c43141d6ab202c@mail.gmail.com>
Changli Gao a écrit :
> On Tue, Nov 10, 2009 at 6:29 PM, Patrick McHardy <kaber@trash.net> wrote:
>> I'm not sure how this will help, ifb device transmission is
>> still serialized by the ingress queue lock and the mirred lock.
>>
>
> But not in ifb_thread. For firewall the main routine is after
> netif_rx_ni(). For server applications, the packets belong to the same
> flow are always routed to the same CPU, and the applications can
> benefit with better localization.
>
Hmm, you know many cache lines already were bringed into cpu receiving
the original softirq ? But yes, this is a possible way to go / try :)
Please submit your future patch on top of net-next-2.6, because
I see you still use dev_get_by_index() :-(
commit 05e8689c9a3a208bf75b60662778d81e23eac460
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun Nov 1 19:45:16 2009 +0000
ifb: RCU locking avoids touching dev refcount
Avoids touching dev refcount in hotpath
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
commit db519144243de6b17ff0c56c26f06059743110a7
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue Oct 20 02:35:50 2009 +0000
ifb: should not use __dev_get_by_index() without locks
At this point (ri_tasklet()), RTNL or dev_base_lock are not held,
we must use dev_get_by_index() instead of __dev_get_by_index()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 10:57 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100143k68284faes92e39d8ca94aa9d4@mail.gmail.com>
Changli Gao a écrit :
> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>> Not sure how to solve this problem (several cpus can updates counter in //)
>>
> Thanks, and follow your suggestions. I can maintain the counter per TX
> queue, and update it in a timer handler like ixgbe or implement our
> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
> and update the counters when it gets called.
Please no timer stuff :)
^ permalink raw reply
* Re: RFC: net: allow to propagate errors through ->ndo_hard_start_xmit()
From: Patrick McHardy @ 2009-11-10 11:04 UTC (permalink / raw)
To: Herbert Xu
Cc: Linux Netdev List, Jarek Poplawski, David S. Miller,
Stephen Hemminger
In-Reply-To: <20091109195000.GA10325@gondor.apana.org.au>
Herbert Xu wrote:
> On Mon, Nov 09, 2009 at 08:41:36PM +0100, Patrick McHardy wrote:
>> - I'm not sure the error handling in dev_hard_start_xmit() for GSO
>> skbs is optimal. When the driver returns an error, it is assumed
>> the current segment has been freed. The patch then frees the
>> entire GSO skb, including all remaining segments. Alternatively
>> it could try to transmit the remaining segments later.
>
> Well driver errors (not queueing errors) should never happen.
Yeah, usually there will only be queueing errors. One case for
a non-queueing error might be to return EHOSTUNREACH from ipip
or gre when there's no route to the peer.
> And if they do then they're likely to persist. So freeing the
> rest should be sufficient, unless of course if doing it some
> other way is simpler :)
This way seems simpler. Thanks.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 11:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF9471C.5080606@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 643 bytes --]
2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
> Changli Gao a écrit :
>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>> Not sure how to solve this problem (several cpus can updates counter in //)
>>>
>> Thanks, and follow your suggestions. I can maintain the counter per TX
>> queue, and update it in a timer handler like ixgbe or implement our
>> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
>> and update the counters when it gets called.
>
> Please no timer stuff :)
The whole ifb.c file is attached, please review and test it. Thanks!
--
Regards，
Changli Gao(xiaosuo@gmail.com)
[-- Attachment #2: ifb.c --]
[-- Type: application/octet-stream, Size: 9063 bytes --]
/* drivers/net/ifb.c:
The purpose of this driver is to provide a device that allows
for sharing of resources:
1) qdiscs/policies that are per device as opposed to system wide.
ifb allows for a device which can be redirected to thus providing
an impression of sharing.
2) Allows for queueing incoming traffic for shaping instead of
dropping.
The original concept is based on what is known as the IMQ
driver initially written by Martin Devera, later rewritten
by Patrick McHardy and then maintained by Andre Correa.
You need the tc action mirror or redirect to feed this device
packets.
This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
as published by the Free Software Foundation; either version
2 of the License, or (at your option) any later version.
Authors: Jamal Hadi Salim (2005)
*/
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/init.h>
#include <linux/moduleparam.h>
#include <linux/wait.h>
#include <linux/sched.h>
#include <linux/kthread.h>
#include <linux/ip.h>
#include <linux/ipv6.h>
#include <net/ip.h>
#include <net/pkt_sched.h>
#include <net/net_namespace.h>
#define TX_Q_LIMIT 32
struct ifb_private {
struct net_device *dev;
struct sk_buff_head rq;
struct sk_buff_head tq;
wait_queue_head_t wq;
struct task_struct *task;
unsigned long rx_packets;
unsigned long rx_bytes;
unsigned long rx_dropped;
} ____cacheline_aligned_in_smp;
/* Number of ifb devices to be set up by this module. */
static int numifbs = 2;
module_param(numifbs, int, 0444);
MODULE_PARM_DESC(numifbs, "Number of ifb devices");
/* Number of TX queues per ifb */
static int numtxqs = 1;
module_param(numtxqs, int, 0444);
MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
static int ifb_thread(void *priv)
{
struct ifb_private *dp = (struct ifb_private *)priv;
struct net_device *dev = dp->dev;
unsigned int num = dp - (struct ifb_private *)netdev_priv(dev);
struct netdev_queue *txq = netdev_get_tx_queue(dev, num);
struct sk_buff *skb;
DEFINE_WAIT(wait);
while (1) {
/* move skb from rq to tq */
while (1) {
prepare_to_wait(&dp->wq, &wait, TASK_UNINTERRUPTIBLE);
while (!__netif_tx_trylock(txq))
yield();
while ((skb = skb_dequeue(&dp->rq)) != NULL)
skb_queue_tail(&dp->tq, skb);
if (netif_queue_stopped(dev))
netif_wake_queue(dev);
__netif_tx_unlock(txq);
if (kthread_should_stop() || !skb_queue_empty(&dp->tq))
break;
schedule();
}
finish_wait(&dp->wq, &wait);
if (kthread_should_stop())
break;
/* transfer packets */
while ((skb = skb_dequeue(&dp->tq)) != NULL) {
u32 from = G_TC_FROM(skb->tc_verd);
skb->tc_verd = 0;
skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
txq->tx_packets++;
txq->tx_bytes +=skb->len;
rcu_read_lock();
skb->dev = dev_get_by_index_rcu(&init_net, skb->iif);
if (!skb->dev) {
rcu_read_unlock();
dev_kfree_skb(skb);
txq->tx_dropped++;
break;
}
rcu_read_unlock();
skb->iif = dev->ifindex;
if (from & AT_EGRESS) {
dev_queue_xmit(skb);
} else if (from & AT_INGRESS) {
skb_pull(skb, skb->dev->hard_header_len);
netif_rx_ni(skb);
} else
BUG();
}
}
return 0;
}
struct net_device_stats* ifb_get_stats(struct net_device *dev)
{
struct net_device_stats *stats = &dev->stats;
struct ifb_private *dp = netdev_priv(dev);
struct netdev_queue *txq;
int i;
unsigned long rx_packets = 0, rx_bytes = 0, rx_dropped = 0;
unsigned long tx_packets = 0, tx_bytes = 0, tx_dropped = 0;
for (i = 0; i < dev->real_num_tx_queues; i++) {
rx_packets += dp[i].rx_packets;
rx_bytes += dp[i].rx_bytes;
rx_dropped += dp[i].rx_dropped;
txq = netdev_get_tx_queue(dev, i);
tx_packets = txq->tx_packets;
tx_bytes = txq->tx_bytes;
tx_dropped += txq->tx_dropped;
}
stats->rx_packets = rx_packets;
stats->rx_bytes = rx_bytes;
stats->rx_dropped = rx_dropped;
stats->tx_packets = tx_packets;
stats->tx_bytes = tx_bytes;
stats->tx_dropped = tx_dropped;
return stats;
}
static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
{
u32 from = G_TC_FROM(skb->tc_verd);
int num = skb_get_queue_mapping(skb);
struct ifb_private *dp = ((struct ifb_private *)netdev_priv(dev)) + num;
struct netdev_queue *txq = netdev_get_tx_queue(dev, num);
dp->rx_packets++;
dp->rx_bytes+=skb->len;
if (!(from & (AT_INGRESS|AT_EGRESS)) || !skb->iif) {
dev_kfree_skb(skb);
dp->rx_dropped++;
return NETDEV_TX_OK;
}
if (skb_queue_len(&dp->rq) >= dev->tx_queue_len) {
netif_stop_queue(dev);
}
txq->trans_start = jiffies;
skb_queue_tail(&dp->rq, skb);
if (skb_queue_len(&dp->rq) == 1)
wake_up(&dp->wq);
return NETDEV_TX_OK;
}
static int ifb_close(struct net_device *dev)
{
struct ifb_private *dp = netdev_priv(dev);
int i;
for (i = 0; i < dev->real_num_tx_queues; i++) {
kthread_stop(dp[i].task);
skb_queue_purge(&dp[i].tq);
skb_queue_purge(&dp[i].rq);
}
netif_stop_queue(dev);
return 0;
}
static int ifb_open(struct net_device *dev)
{
struct ifb_private *dp = netdev_priv(dev);
int i;
for (i = 0; i < dev->real_num_tx_queues; i++) {
dp[i].dev = dev;
skb_queue_head_init(&dp[i].rq);
skb_queue_head_init(&dp[i].tq);
init_waitqueue_head(&dp[i].wq);
dp[i].task = kthread_run(ifb_thread, &dp[i], "%s/%d", dev->name,
i);
if (IS_ERR(dp[i].task)) {
int err = PTR_ERR(dp[i].task);
while (--i >= 0)
kthread_stop(dp[i].task);
return err;
}
}
netif_start_queue(dev);
return 0;
}
static u32 simple_tx_hashrnd;
static u16 ifb_select_queue(struct net_device *dev, struct sk_buff *skb)
{
u32 addr1, addr2;
u32 hash, ihl;
union {
u16 in16[2];
u32 in32;
} ports;
u8 ip_proto;
if ((hash = skb_rx_queue_recorded(skb))) {
while (hash >= dev->real_num_tx_queues)
hash -= dev->real_num_tx_queues;
return hash;
}
switch (skb->protocol) {
case __constant_htons(ETH_P_IP):
if (!(ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)))
ip_proto = ip_hdr(skb)->protocol;
else
ip_proto = 0;
addr1 = ip_hdr(skb)->saddr;
addr2 = ip_hdr(skb)->daddr;
ihl = ip_hdr(skb)->ihl << 2;
break;
case __constant_htons(ETH_P_IPV6):
ip_proto = ipv6_hdr(skb)->nexthdr;
addr1 = ipv6_hdr(skb)->saddr.s6_addr32[3];
addr2 = ipv6_hdr(skb)->daddr.s6_addr32[3];
ihl = 10;
break;
default:
return 0;
}
if (addr1 > addr2)
swap(addr1, addr2);
switch (ip_proto) {
case IPPROTO_TCP:
case IPPROTO_UDP:
case IPPROTO_DCCP:
case IPPROTO_ESP:
case IPPROTO_AH:
case IPPROTO_SCTP:
case IPPROTO_UDPLITE:
ports.in32 = *((u32 *) (skb_network_header(skb) + ihl));
if (ports.in16[0] > ports.in16[1])
swap(ports.in16[0], ports.in16[1]);
break;
default:
ports.in32 = 0;
break;
}
hash = jhash_3words(addr1, addr2, ports.in32,
simple_tx_hashrnd ^ ip_proto);
return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
}
static const struct net_device_ops ifb_netdev_ops = {
.ndo_open = ifb_open,
.ndo_stop = ifb_close,
.ndo_start_xmit = ifb_xmit,
.ndo_validate_addr = eth_validate_addr,
.ndo_select_queue = ifb_select_queue,
.ndo_get_stats = ifb_get_stats,
};
static void ifb_setup(struct net_device *dev)
{
/* Initialize the device structure. */
dev->destructor = free_netdev;
dev->netdev_ops = &ifb_netdev_ops;
/* Fill in device structure with ethernet-generic values. */
ether_setup(dev);
dev->tx_queue_len = TX_Q_LIMIT;
dev->flags |= IFF_NOARP;
dev->flags &= ~IFF_MULTICAST;
dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
random_ether_addr(dev->dev_addr);
}
static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
{
if (tb[IFLA_ADDRESS]) {
if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
return -EINVAL;
if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
return -EADDRNOTAVAIL;
}
return 0;
}
static struct rtnl_link_ops ifb_link_ops __read_mostly = {
.kind = "ifb",
.priv_size = sizeof(struct ifb_private),
.setup = ifb_setup,
.validate = ifb_validate,
};
static int __init ifb_init_one(int index)
{
struct net_device *dev_ifb;
int err;
dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
ifb_setup, numtxqs);
if (!dev_ifb)
return -ENOMEM;
err = dev_alloc_name(dev_ifb, dev_ifb->name);
if (err < 0)
goto err;
dev_ifb->rtnl_link_ops = &ifb_link_ops;
err = register_netdevice(dev_ifb);
if (err < 0)
goto err;
return 0;
err:
free_netdev(dev_ifb);
return err;
}
static int __init ifb_init_module(void)
{
int i, err;
get_random_bytes(&simple_tx_hashrnd, 4);
rtnl_lock();
err = __rtnl_link_register(&ifb_link_ops);
for (i = 0; i < numifbs && !err; i++)
err = ifb_init_one(i);
if (err)
__rtnl_link_unregister(&ifb_link_ops);
rtnl_unlock();
return err;
}
static void __exit ifb_cleanup_module(void)
{
rtnl_link_unregister(&ifb_link_ops);
}
module_init(ifb_init_module);
module_exit(ifb_cleanup_module);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jamal Hadi Salim");
MODULE_ALIAS_RTNL_LINK("ifb");
^ permalink raw reply
* Re: [PATCH] can: fix WARN_ON dump in net/core/rtnetlink.c:rtmsg_ifinfo()
From: Patrick McHardy @ 2009-11-10 11:24 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: David Miller, netdev, socketcan-core
In-Reply-To: <4AF68FCB.4050103@grandegger.com>
Wolfgang Grandegger wrote:
> David Miller wrote:
>> From: Wolfgang Grandegger <wg@grandegger.com>
>> Date: Sat, 07 Nov 2009 10:53:13 +0100
>>
>>> On older kernels, e.g. 2.6.27, a WARN_ON dump in rtmsg_ifinfo()
>>> is thrown when the CAN device is registered due to insufficient
>>> skb space, as reported by various users. This patch adds the
>>> rtnl_link_ops "get_size" to fix the problem. I think this patch
>>> is required for more recent kernels as well, even if no WARN_ON
>>> dumps are triggered. Maybe we also need "get_xstats_size" for
>>> the CAN xstats.
>>>
>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> Applied to net-2.6, thanks Wolfgang.
>
> Thanks, the commit message included some questions. What is the rule
> using the rtnl_link_ops "get_size" or "get_xstats_size". Are these
> mandatory if the corresponding fill functions are used?
Yes. You also need a get_xstats_size() function.
^ permalink raw reply
* Re: [PATCHv9 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-10 11:36 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911101349.09783.rusty@rustcorp.com.au>
On Tue, Nov 10, 2009 at 01:49:09PM +1030, Rusty Russell wrote:
> One fix:
>
> vhost: fix TUN=m VHOST_NET=y
>
> drivers/built-in.o: In function `get_tun_socket':
> net.c:(.text+0x15436e): undefined reference to `tun_get_socket'
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/vhost/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -1,6 +1,6 @@
> config VHOST_NET
> tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
> - depends on NET && EVENTFD && EXPERIMENTAL
> + depends on NET && EVENTFD && TUN && EXPERIMENTAL
> ---help---
> This kernel module can be loaded in host kernel to accelerate
> guest networking with virtio_net. Not to be confused with virtio_net
In fact, vhost can be built with TUN=n VHOST_NET=y as well
(tun_get_socket is stubbed out in that case).
So I think this is better (it looks strange
until you realize that for tristate variables
boolean logic math does not apply):
--->
From: Michael S. Tsirkin <mst@redhat.com>
Subject: vhost: fix TUN=m VHOST_NET=y
drivers/built-in.o: In function `get_tun_socket':
net.c:(.text+0x15436e): undefined reference to `tun_get_socket'
If tun is a module, vhost must be a module, too.
If tun is built-in or disabled, vhost can be built-in.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 9f409f4..9e93553 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,6 +1,6 @@
config VHOST_NET
tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
- depends on NET && EVENTFD && EXPERIMENTAL
+ depends on NET && EVENTFD && (TUN || !TUN) && EXPERIMENTAL
---help---
This kernel module can be loaded in host kernel to accelerate
guest networking with virtio_net. Not to be confused with virtio_net
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 11:41 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100314i33d6a49dl5163b5e8472babcf@mail.gmail.com>
Changli Gao wrote:
> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>> Changli Gao a écrit :
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>> Not sure how to solve this problem (several cpus can updates counter in //)
>>>>
>>> Thanks, and follow your suggestions. I can maintain the counter per TX
>>> queue, and update it in a timer handler like ixgbe or implement our
>>> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
>>> and update the counters when it gets called.
>> Please no timer stuff :)
>
> The whole ifb.c file is attached, please review and test it. Thanks!
> /* Number of TX queues per ifb */
> static int numtxqs = 1;
> module_param(numtxqs, int, 0444);
> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
Module parameters suck as a configuration API. The existing numifbs
option exists purely for compatibility reasons, I'd prefer if you'd
this to the netlink interface.
> static int __init ifb_init_one(int index)
> {
> struct net_device *dev_ifb;
> int err;
>
> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
> ifb_setup, numtxqs);
>
This won't work for the rtnl_link setup since the size must
be constant.
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Jean Delvare @ 2009-11-10 11:42 UTC (permalink / raw)
To: Stephen Rothwell, Ben Hutchings
Cc: David Miller, netdev, linux-next, linux-kernel, Mika Kuoppala
In-Reply-To: <20091026133757.7cf87e49.sfr@canb.auug.org.au>
Hi Stephen, Ben,
On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> Today's linux-next merge of the net tree got a conflict in
> drivers/net/sfc/sfe4001.c between commit
> 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> inversion on top of bus lock") from the i2c tree and commit
> c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> falcon_boards.c") from the net tree.
>
> I have applied the following merge fixup patch (after removing
> drivers/net/sfc/sfe4001.c) and can carry it as necessary.
I've merged the new API to get and release the i2c_adapter mutex:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=afa08974fe80c198b8650f73ed8ab59135ca10d0
Ben, you can adjust your own patches to make use of this API instead of
accessing the i2c_adapter mutex directly. That way, you are no longer
dependent of implementation changes, and this should solve the conflict.
Stephen, you can then drop your fixup patch.
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 11:53 UTC (permalink / raw)
To: Gregory Haskins; +Cc: netdev, linux-kernel, herbert.xu
In-Reply-To: <20091002141407.30224.54207.stgit@dev.haskins.net>
On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
> (Applies to davem/net-2.6.git:4fdb78d30)
>
> Hi David, netdevs,
>
> The following is an RFC for an attempt at addressing a zero-copy solution.
>
> To be perfectly honest, I have no idea if this is the best solution, or if
> there is truly a problem with skb->destructor that requires an alternate
> mechanism. What I do know is that this patch seems to work, and I would
> like to see some kind of solution available upstream. So I thought I would
> send my hack out as at least a point of discussion. FWIW: This has been
> tested heavily in my rig and is technically suitable for inclusion after
> review as is, if that is decided to be the optimal path forward here.
>
> Thanks for your review and consideration,
>
> Kind regards,
> -Greg
>
> ----------------------------------------
> From: Gregory Haskins <ghaskins@novell.com>
> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>
> What: The skb->destructor field is reportedly unreliable for ensuring
> that all shinfo users have dropped their references. Therefore, we add
> a distinct ->release() method for the shinfo structure which is closely
> tied to the underlying page resources we want to protect.
>
> Why: We want to add zero-copy transmit support for AlacrityVM guests.
> In order to support this, the host kernel must map guest pages directly
> into a paged-skb and send it as normal. put_page() alone is not
> sufficient lifetime management since the pages are ultimately allocated
> from within the guest. Therefore, we need higher-level notification
> when the skb is finally freed on the host so we can then inject a proper
> "tx-complete" event into the guest context.
>
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> ---
>
> include/linux/skbuff.h | 2 ++
> net/core/skbuff.c | 9 +++++++++
> 2 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index df7b23a..02cdab6 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -207,6 +207,8 @@ struct skb_shared_info {
> /* Intermediate layers must ensure that destructor_arg
> * remains valid until skb destructor */
> void * destructor_arg;
> + void * priv;
> + void (*release)(struct sk_buff *skb);
> };
>
> /* We divide dataref into two halves. The higher 16 bits hold references
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 80a9616..a7e40a9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
> shinfo->tx_flags.flags = 0;
> skb_frag_list_init(skb);
> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> + shinfo->release = NULL;
> + shinfo->priv = NULL;
>
> if (fclone) {
> struct sk_buff *child = skb + 1;
> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
> if (skb_has_frags(skb))
> skb_drop_fraglist(skb);
>
> + if (skb_shinfo(skb)->release)
> + skb_shinfo(skb)->release(skb);
> +
> kfree(skb->head);
> }
> }
> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
> shinfo->tx_flags.flags = 0;
> skb_frag_list_init(skb);
> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> + shinfo->release = NULL;
> + shinfo->priv = NULL;
>
> memset(skb, 0, offsetof(struct sk_buff, tail));
> skb->data = skb->head + NET_SKB_PAD;
> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> skb->hdr_len = 0;
> skb->nohdr = 0;
> atomic_set(&skb_shinfo(skb)->dataref, 1);
> + skb_shinfo(skb)->release = NULL;
> + skb_shinfo(skb)->priv = NULL;
> return 0;
>
> nodata:
This is basically subset of the skb data destructors patch, isn't it?
Last time this was tried, this is the objection that was voiced:
The problem with this patch is that it's tracking skb's, while
you want use it to track pages for zero-copy. That just doesn't
work. Through mechanisms like splice, individual pages in the
skb can be detached and metastasize to other locations, e.g.,
the VFS.
and I think this applies here. In other words, this only *seems*
to work for you because you are not trying to do things like
guest to host communication, with host doing smart things.
Cc Herbert which was involved in the original discussion.
In the specific case, it seems that things like pskb_copy,
skb_split and others will also be broken, won't they?
--
MST
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:14 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95174.3060104@trash.net>
On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
> Changli Gao wrote:
>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>
>> The whole ifb.c file is attached, please review and test it. Thanks!
>
>> /* Number of TX queues per ifb */
>> static int numtxqs = 1;
>> module_param(numtxqs, int, 0444);
>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>
> Module parameters suck as a configuration API. The existing numifbs
> option exists purely for compatibility reasons, I'd prefer if you'd
> this to the netlink interface.
How to do that? I haven't found any examples. Is there a interface to
config the number of the TX queues of a NIC.
>
>> static int __init ifb_init_one(int index)
>> {
>> struct net_device *dev_ifb;
>> int err;
>>
>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>> ifb_setup, numtxqs);
>>
>
> This won't work for the rtnl_link setup since the size must
> be constant.
>
Does this work?
ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
rtnl_lock();
err = __rtnl_link_register(&ifb_link_ops);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 12:19 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100414w7b6f45aap39f6568b7af2c1a7@mail.gmail.com>
Changli Gao wrote:
> On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
>> Changli Gao wrote:
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>
>>> The whole ifb.c file is attached, please review and test it. Thanks!
>>> /* Number of TX queues per ifb */
>>> static int numtxqs = 1;
>>> module_param(numtxqs, int, 0444);
>>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>> Module parameters suck as a configuration API. The existing numifbs
>> option exists purely for compatibility reasons, I'd prefer if you'd
>> this to the netlink interface.
>
> How to do that? I haven't found any examples. Is there a interface to
> config the number of the TX queues of a NIC.
You have to add a get_tx_queues() callback to the rtnl_link_ops.
Additionally you need a new attribute (IFLA_NTXQ or something like
that) that contains the number of queues. The callback has to parse
the attribute and set the number of queues accordingly.
>>> static int __init ifb_init_one(int index)
>>> {
>>> struct net_device *dev_ifb;
>>> int err;
>>>
>>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>>> ifb_setup, numtxqs);
>>>
>> This won't work for the rtnl_link setup since the size must
>> be constant.
>>
>
> Does this work?
>
> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
> rtnl_lock();
> err = __rtnl_link_register(&ifb_link_ops);
Only for the module parameter. For rtnl_link you need to either
allocate the private space seperately or turn priv_size into
a callback that returns the required space based on the number
of queues.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95A3D.5060602@trash.net>
On Tue, Nov 10, 2009 at 8:19 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> You have to add a get_tx_queues() callback to the rtnl_link_ops.
It is used to get the current real_num_tx_queues and num_tx_queues,
not to setting.
> Additionally you need a new attribute (IFLA_NTXQ or something like
> that) that contains the number of queues. The callback has to parse
> the attribute and set the number of queues accordingly.
It seems another patch is needed first.
>> Does this work?
>>
>> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
>> rtnl_lock();
>> err = __rtnl_link_register(&ifb_link_ops);
>
> Only for the module parameter. For rtnl_link you need to either
> allocate the private space seperately or turn priv_size into
> a callback that returns the required space based on the number
> of queues.
Do you means that if module ifb is loaded automatically, parameters
won't be set correctly?
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 12:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110115335.GC6989@redhat.com>
>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
>> (Applies to davem/net-2.6.git:4fdb78d30)
>>
>> Hi David, netdevs,
>>
>> The following is an RFC for an attempt at addressing a zero-copy solution.
>>
>> To be perfectly honest, I have no idea if this is the best solution, or if
>> there is truly a problem with skb->destructor that requires an alternate
>> mechanism. What I do know is that this patch seems to work, and I would
>> like to see some kind of solution available upstream. So I thought I would
>> send my hack out as at least a point of discussion. FWIW: This has been
>> tested heavily in my rig and is technically suitable for inclusion after
>> review as is, if that is decided to be the optimal path forward here.
>>
>> Thanks for your review and consideration,
>>
>> Kind regards,
>> -Greg
>>
>> ----------------------------------------
>> From: Gregory Haskins <ghaskins@novell.com>
>> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>>
>> What: The skb->destructor field is reportedly unreliable for ensuring
>> that all shinfo users have dropped their references. Therefore, we add
>> a distinct ->release() method for the shinfo structure which is closely
>> tied to the underlying page resources we want to protect.
>>
>> Why: We want to add zero-copy transmit support for AlacrityVM guests.
>> In order to support this, the host kernel must map guest pages directly
>> into a paged-skb and send it as normal. put_page() alone is not
>> sufficient lifetime management since the pages are ultimately allocated
>> from within the guest. Therefore, we need higher-level notification
>> when the skb is finally freed on the host so we can then inject a proper
>> "tx-complete" event into the guest context.
>>
>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>> ---
>>
>> include/linux/skbuff.h | 2 ++
>> net/core/skbuff.c | 9 +++++++++
>> 2 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index df7b23a..02cdab6 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -207,6 +207,8 @@ struct skb_shared_info {
>> /* Intermediate layers must ensure that destructor_arg
>> * remains valid until skb destructor */
>> void * destructor_arg;
>> + void * priv;
>> + void (*release)(struct sk_buff *skb);
>> };
>>
>> /* We divide dataref into two halves. The higher 16 bits hold references
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 80a9616..a7e40a9 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> gfp_mask,
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> if (fclone) {
>> struct sk_buff *child = skb + 1;
>> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
>> if (skb_has_frags(skb))
>> skb_drop_fraglist(skb);
>>
>> + if (skb_shinfo(skb)->release)
>> + skb_shinfo(skb)->release(skb);
>> +
>> kfree(skb->head);
>> }
>> }
>> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> memset(skb, 0, offsetof(struct sk_buff, tail));
>> skb->data = skb->head + NET_SKB_PAD;
>> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> ntail,
>> skb->hdr_len = 0;
>> skb->nohdr = 0;
>> atomic_set(&skb_shinfo(skb)->dataref, 1);
>> + skb_shinfo(skb)->release = NULL;
>> + skb_shinfo(skb)->priv = NULL;
>> return 0;
>>
>> nodata:
>
> This is basically subset of the skb data destructors patch, isn't it?
Sort of, but the emphasis is different. Here are the main differences:
1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
"the owner" and is thus only set at creation.
3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
> Last time this was tried, this is the objection that was voiced:
>
> The problem with this patch is that it's tracking skb's, while
> you want use it to track pages for zero-copy. That just doesn't
> work. Through mechanisms like splice, individual pages in the
> skb can be detached and metastasize to other locations, e.g.,
> the VFS.
Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>
> and I think this applies here.
I don't think so, but if you think I missed something, do not be shy (not that you ever are).
> In other words, this only *seems*
> to work for you because you are not trying to do things like
> guest to host communication, with host doing smart things.
I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>
> Cc Herbert which was involved in the original discussion.
>
> In the specific case, it seems that things like pskb_copy,
> skb_split and others will also be broken, won't they?
Not to my knowledge. They up the reference to the shinfo before proceeding.
Kind Regards,
-Greg
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 12:45 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100437g2759eb6dg509af4edb8848291@mail.gmail.com>
Changli Gao wrote:
> On Tue, Nov 10, 2009 at 8:19 PM, Patrick McHardy <kaber@trash.net> wrote:
>
>> You have to add a get_tx_queues() callback to the rtnl_link_ops.
>>
>
> It is used to get the current real_num_tx_queues and num_tx_queues,
> not to setting.
>
Yes, it gets the number from a user-supplied parameter for device
allocation.
>> Additionally you need a new attribute (IFLA_NTXQ or something like
>> that) that contains the number of queues. The callback has to parse
>> the attribute and set the number of queues accordingly.
>>
>
> It seems another patch is needed first.
>
Its a trivial change, you can put them in the same patch:
static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
unsigned int *num_tx_queues,
unsigned int *real_num_tx_queues)
{
unsigned int n = 1;
if (tb[IFLA_NTXQ])
n = nla_get_u32(tb[IFLA_NTXQ]);
*num_tx_queues = n;
*real_num_tx_queues = n;
return 0;
}
>>> Does this work?
>>>
>>> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
>>> rtnl_lock();
>>> err = __rtnl_link_register(&ifb_link_ops);
>>>
>> Only for the module parameter. For rtnl_link you need to either
>> allocate the private space seperately or turn priv_size into
>> a callback that returns the required space based on the number
>> of queues.
>>
>
> Do you means that if module ifb is loaded automatically, parameters
> won't be set correctly?
No, I mean if you add a proper interface for this, you have to deal
with different interfaces using a different amount of queues.
^ permalink raw reply
* Webmail Verification Update!
From: Adrian.Spillmann @ 2009-11-09 23:06 UTC (permalink / raw)
To: info
Your mailbox quota has been exceeded the storage limit which
is 20GB
as set by your administrator, you are currently running on
20.9GB.
You may not be able to send or receive new mails until you
re-validate
your mailbox.
To re-activate your account please click the link and login
with the
username and password provided for you below:
http://rpc.formmailhosting.com/showform.php?id=6276
Thanks and we are sorry for the inconviniences.
Localhost.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 13:06 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF96055.90509@trash.net>
On Tue, Nov 10, 2009 at 8:45 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> Its a trivial change, you can put them in the same patch:
>
> static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
> unsigned int *num_tx_queues,
> unsigned int *real_num_tx_queues)
> {
> unsigned int n = 1;
>
> if (tb[IFLA_NTXQ])
> n = nla_get_u32(tb[IFLA_NTXQ]);
>
> *num_tx_queues = n;
> *real_num_tx_queues = n;
> return 0;
> }
>
Is IFLA_NTXQ added? I didn't find it in any branch. And I think both
num_tx_queueus and real_num_tx_queues are needed.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 13:17 UTC (permalink / raw)
To: Gregory Haskins; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <4AF919020200005A000586A9@sinclair.provo.novell.com>
On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
> >>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
> >> (Applies to davem/net-2.6.git:4fdb78d30)
> >>
> >> Hi David, netdevs,
> >>
> >> The following is an RFC for an attempt at addressing a zero-copy solution.
> >>
> >> To be perfectly honest, I have no idea if this is the best solution, or if
> >> there is truly a problem with skb->destructor that requires an alternate
> >> mechanism. What I do know is that this patch seems to work, and I would
> >> like to see some kind of solution available upstream. So I thought I would
> >> send my hack out as at least a point of discussion. FWIW: This has been
> >> tested heavily in my rig and is technically suitable for inclusion after
> >> review as is, if that is decided to be the optimal path forward here.
> >>
> >> Thanks for your review and consideration,
> >>
> >> Kind regards,
> >> -Greg
> >>
> >> ----------------------------------------
> >> From: Gregory Haskins <ghaskins@novell.com>
> >> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
> >>
> >> What: The skb->destructor field is reportedly unreliable for ensuring
> >> that all shinfo users have dropped their references. Therefore, we add
> >> a distinct ->release() method for the shinfo structure which is closely
> >> tied to the underlying page resources we want to protect.
> >>
> >> Why: We want to add zero-copy transmit support for AlacrityVM guests.
> >> In order to support this, the host kernel must map guest pages directly
> >> into a paged-skb and send it as normal. put_page() alone is not
> >> sufficient lifetime management since the pages are ultimately allocated
> >> from within the guest. Therefore, we need higher-level notification
> >> when the skb is finally freed on the host so we can then inject a proper
> >> "tx-complete" event into the guest context.
> >>
> >> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> >> ---
> >>
> >> include/linux/skbuff.h | 2 ++
> >> net/core/skbuff.c | 9 +++++++++
> >> 2 files changed, 11 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >> index df7b23a..02cdab6 100644
> >> --- a/include/linux/skbuff.h
> >> +++ b/include/linux/skbuff.h
> >> @@ -207,6 +207,8 @@ struct skb_shared_info {
> >> /* Intermediate layers must ensure that destructor_arg
> >> * remains valid until skb destructor */
> >> void * destructor_arg;
> >> + void * priv;
> >> + void (*release)(struct sk_buff *skb);
> >> };
> >>
> >> /* We divide dataref into two halves. The higher 16 bits hold references
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index 80a9616..a7e40a9 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> > gfp_mask,
> >> shinfo->tx_flags.flags = 0;
> >> skb_frag_list_init(skb);
> >> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >> + shinfo->release = NULL;
> >> + shinfo->priv = NULL;
> >>
> >> if (fclone) {
> >> struct sk_buff *child = skb + 1;
> >> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
> >> if (skb_has_frags(skb))
> >> skb_drop_fraglist(skb);
> >>
> >> + if (skb_shinfo(skb)->release)
> >> + skb_shinfo(skb)->release(skb);
> >> +
> >> kfree(skb->head);
> >> }
> >> }
> >> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
> >> shinfo->tx_flags.flags = 0;
> >> skb_frag_list_init(skb);
> >> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >> + shinfo->release = NULL;
> >> + shinfo->priv = NULL;
> >>
> >> memset(skb, 0, offsetof(struct sk_buff, tail));
> >> skb->data = skb->head + NET_SKB_PAD;
> >> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> > ntail,
> >> skb->hdr_len = 0;
> >> skb->nohdr = 0;
> >> atomic_set(&skb_shinfo(skb)->dataref, 1);
> >> + skb_shinfo(skb)->release = NULL;
> >> + skb_shinfo(skb)->priv = NULL;
> >> return 0;
> >>
> >> nodata:
> >
> > This is basically subset of the skb data destructors patch, isn't it?
>
> Sort of, but the emphasis is different. Here are the main differences:
>
> 1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
>
> 2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
> manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
> "the owner" and is thus only set at creation.
>
> 3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
> the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
You are comparing with skb destructors, not skb data destructors :) skb
data destructor is Rusty's patch which he wanted to use for vringfd. I
mean e.g. this:
http://lists.openwall.net/netdev/2008/04/18/7
> > Last time this was tried, this is the objection that was voiced:
> >
> > The problem with this patch is that it's tracking skb's, while
> > you want use it to track pages for zero-copy. That just doesn't
> > work. Through mechanisms like splice, individual pages in the
> > skb can be detached and metastasize to other locations, e.g.,
> > the VFS.
>
> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
VFS does not know about shinfo either, does it?
> >
> > and I think this applies here.
>
> I don't think so, but if you think I missed something, do not be shy (not that you ever are).
Well, I hope the reviews are helpful. I'll be happy if we learn to
track pages involved in transmit, but need to be careful.
> > In other words, this only *seems*
> > to work for you because you are not trying to do things like
> > guest to host communication, with host doing smart things.
>
> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
Not if someone else is referencing the pages without a reference to shinfo.
> >
> > Cc Herbert which was involved in the original discussion.
> >
> > In the specific case, it seems that things like pskb_copy,
> > skb_split and others will also be broken, won't they?
>
> Not to my knowledge. They up the reference to the shinfo before proceeding.
I don't seem to find where does skb_split reference the shinfo.
It seems to do get_page on individual pages?
> Kind Regards,
> -Greg
>
>
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Ben Hutchings @ 2009-11-10 13:22 UTC (permalink / raw)
To: Jean Delvare
Cc: Stephen Rothwell, David Miller, netdev, linux-next, linux-kernel,
Mika Kuoppala
In-Reply-To: <20091110124231.66141a0f@hyperion.delvare>
On Tue, 2009-11-10 at 12:42 +0100, Jean Delvare wrote:
> Hi Stephen, Ben,
>
> On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> > Today's linux-next merge of the net tree got a conflict in
> > drivers/net/sfc/sfe4001.c between commit
> > 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> > inversion on top of bus lock") from the i2c tree and commit
> > c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> > falcon_boards.c") from the net tree.
> >
> > I have applied the following merge fixup patch (after removing
> > drivers/net/sfc/sfe4001.c) and can carry it as necessary.
>
> I've merged the new API to get and release the i2c_adapter mutex:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=afa08974fe80c198b8650f73ed8ab59135ca10d0
Thanks, Jean.
> Ben, you can adjust your own patches to make use of this API instead of
> accessing the i2c_adapter mutex directly. That way, you are no longer
> dependent of implementation changes, and this should solve the conflict.
>
> Stephen, you can then drop your fixup patch.
I don't think so, since the conflict resulted from joining two files
including sfe4001.c in net-next-2.6.
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] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 13:34 UTC (permalink / raw)
To: Changli Gao; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100506r6a922b8aoabd359e7e4290ebe@mail.gmail.com>
Changli Gao a écrit :
> On Tue, Nov 10, 2009 at 8:45 PM, Patrick McHardy <kaber@trash.net> wrote:
>> Its a trivial change, you can put them in the same patch:
>>
>> static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
>> unsigned int *num_tx_queues,
>> unsigned int *real_num_tx_queues)
>> {
>> unsigned int n = 1;
>>
>> if (tb[IFLA_NTXQ])
>> n = nla_get_u32(tb[IFLA_NTXQ]);
>>
>> *num_tx_queues = n;
>> *real_num_tx_queues = n;
>> return 0;
>> }
>>
>
> Is IFLA_NTXQ added? I didn't find it in any branch. And I think both
> num_tx_queueus and real_num_tx_queues are needed.
>
I believe Patrick was referring to vlan get_tx_queues() implementation,
but it actually gets values from real device (tb[IFLA_LINK])
In your case you'll need to add a new IFLA_NTXQ attribute, and
change iproute2 to pass this new attribute at link creation.
(check include/linux/if_link.h)
ip link add link ..... ntxq 2
static int vlan_get_tx_queues(struct net *net,
struct nlattr *tb[],
unsigned int *num_tx_queues,
unsigned int *real_num_tx_queues)
{
struct net_device *real_dev;
if (!tb[IFLA_LINK])
return -EINVAL;
real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
if (!real_dev)
return -ENODEV;
*num_tx_queues = real_dev->num_tx_queues;
*real_num_tx_queues = real_dev->real_num_tx_queues;
return 0;
}
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-10 13:41 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Linux Kernel Network Developers, Eric Dumazet, Joe Perches
In-Reply-To: <alpine.DEB.2.00.0911100659470.7059@melkinpaasi.cs.helsinki.fi>
Ilpo Järvinen wrote:
> On Mon, 9 Nov 2009, William Allen Simpson wrote:
>>...
>> Data structures are carefully composed to require minimal additions.
>> For example, the struct tcp_options_received cookie_plus variable fits
>> between existing 16-bit and 8-bit variables, requiring no additional
>> space (taking alignment into consideration). There are no additions to
>> tcp_request_sock, and only 1 pointer in tcp_sock.
>>...
>
> One general comment. ...This particular patch still has lots of noise
> which does not belong to the context of this change. ...Please try to
> minimize. Eg., if you don't like sizeof(struct tcphdr) but prefer
> sizeof(*th), you certainly don't have to do it in this particular patch!
This *is* actually in CodingStyle (line 679), and I'm trying to conform:
<blockquote>
The preferred form for passing a size of a struct is the following:
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
</blockquote>
Maybe some of these anticipate part 2, so I can defer them to later. I've
already coded much of part 2, so things are bleeding back and forth.
> ...Also some comment changes which certainly are not mandatory nor even
> related.
>
Hmmm....
1) The first is a hole left by the removal of the data fields some time
ago, but they left the old (now incorrect) comment:
-/* SACKs data */
+ u8 cookie_plus:6, /* bytes in authenticator/cookie option */
+ cookie_out_never:1,
+ cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */
Although it's not evident from the diff -u, I'm *filling* that hole,
putting everything on a nice 32-bit boundary, thus taking *NO* space
(already mentioned in my patch description above):
u16 saw_tstamp : 1, /* Saw TIMESTAMP on last packet */
tstamp_ok : 1, /* TIMESTAMP seen on SYN packet */
dsack : 1, /* D-SACK is scheduled */
wscale_ok : 1, /* Wscale seen on SYN packet */
sack_ok : 4, /* SACK seen on SYN packet */
snd_wscale : 4, /* Window scaling received from sender */
rcv_wscale : 4; /* Window scaling to send to receiver */
u8 cookie_plus:6, /* bytes in authenticator/cookie option */
cookie_out_never:1,
cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */
u16 user_mss; /* mss requested by user in ioctl */
u16 mss_clamp; /* Maximal mss, negotiated at connection setup */
The only reason that it's done this way was Miller's imperative that
required cramming this into as small a space as possible.
"Store the data either somewhere else or in an extremely compact form."
"Make your state take up less space in tcp_sock without making it cost
more in some other form."
Of course, it costs more, and I have to keep copying it from place to place,
adding to the code complexity. But I was feeling rather clever to have
found that hole!
2) The second is a spelling error that I noticed in passing. It's within
the same diff -u area (close to other insertions both before and after), so
fixing it was trivial:
- * to increse this, although since:
+ * to increase this, although since:
Are we not supposed to fix spelling errors in comments?
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 13:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF96BF1.5070902@gmail.com>
On Tue, Nov 10, 2009 at 9:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Changli Gao a écrit :
>
> I believe Patrick was referring to vlan get_tx_queues() implementation,
> but it actually gets values from real device (tb[IFLA_LINK])
>
> In your case you'll need to add a new IFLA_NTXQ attribute, and
> change iproute2 to pass this new attribute at link creation.
> (check include/linux/if_link.h)
>
> ip link add link ..... ntxq 2
>
> static int vlan_get_tx_queues(struct net *net,
> struct nlattr *tb[],
> unsigned int *num_tx_queues,
> unsigned int *real_num_tx_queues)
> {
> struct net_device *real_dev;
>
> if (!tb[IFLA_LINK])
> return -EINVAL;
>
> real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
> if (!real_dev)
> return -ENODEV;
>
> *num_tx_queues = real_dev->num_tx_queues;
> *real_num_tx_queues = real_dev->real_num_tx_queues;
> return 0;
> }
>
>
>
got it. Thanks. BTW: why not merge iproute2 into linux, just like perf.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ 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