* [PATCH net v2] ipv4: fix wildcard search with inet_confirm_addr()
From: Nicolas Dichtel @ 2013-11-13 15:23 UTC (permalink / raw)
To: ja; +Cc: davem, netdev, Nicolas Dichtel
In-Reply-To: <alpine.LFD.2.03.1311082258430.1811@ssi.bg>
Help of this function says: "in_dev: only on this interface, 0=any interface",
but since commit 39a6d0630012 ("[NETNS]: Process inet_confirm_addr in the
correct namespace."), the code supposes that it will never be NULL. This
function is never called with in_dev == NULL, but it's exported and may be used
by an external module.
Because this patch restore the ability to call inet_confirm_addr() with in_dev
== NULL, I partially revert the above commit, as suggested by Julian.
CC: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
This bug was spotted by code review.
v2: fix change in bond_confirm_addr()
partially revert 39a6d0630012 ("[NETNS]: Process inet_confirm_addr in the
correct namespace.")
fix API desc of inet_confirm_addr()
drivers/net/bonding/bonding.h | 4 ++--
include/linux/inetdevice.h | 3 ++-
net/ipv4/arp.c | 4 +++-
net/ipv4/devinet.c | 9 ++++-----
4 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 03cf3fd14490..0ad3f4bd99c0 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -414,8 +414,8 @@ static inline __be32 bond_confirm_addr(struct net_device *dev, __be32 dst, __be3
in_dev = __in_dev_get_rcu(dev);
if (in_dev)
- addr = inet_confirm_addr(in_dev, dst, local, RT_SCOPE_HOST);
-
+ addr = inet_confirm_addr(dev_net(dev), in_dev, dst, local,
+ RT_SCOPE_HOST);
rcu_read_unlock();
return addr;
}
diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 79640e015a86..5870f7060917 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -164,7 +164,8 @@ extern int devinet_ioctl(struct net *net, unsigned int cmd, void __user *);
extern void devinet_init(void);
extern struct in_device *inetdev_by_index(struct net *, int);
extern __be32 inet_select_addr(const struct net_device *dev, __be32 dst, int scope);
-extern __be32 inet_confirm_addr(struct in_device *in_dev, __be32 dst, __be32 local, int scope);
+__be32 inet_confirm_addr(struct net *net, struct in_device *in_dev, __be32 dst,
+ __be32 local, int scope);
extern struct in_ifaddr *inet_ifa_byprefix(struct in_device *in_dev, __be32 prefix, __be32 mask);
static __inline__ int inet_ifa_match(__be32 addr, struct in_ifaddr *ifa)
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 7808093cede6..46c7b4483bcf 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -379,6 +379,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)
{
+ struct net *net = dev_net(in_dev->dev);
int scope;
switch (IN_DEV_ARP_IGNORE(in_dev)) {
@@ -397,6 +398,7 @@ static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)
case 3: /* Do not reply for scope host addresses */
sip = 0;
scope = RT_SCOPE_LINK;
+ in_dev = NULL;
break;
case 4: /* Reserved */
case 5:
@@ -408,7 +410,7 @@ static int arp_ignore(struct in_device *in_dev, __be32 sip, __be32 tip)
default:
return 0;
}
- return !inet_confirm_addr(in_dev, sip, tip, scope);
+ return !inet_confirm_addr(net, in_dev, sip, tip, scope);
}
static int arp_filter(__be32 sip, __be32 tip, struct net_device *dev)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index a1b5bcbd04ae..19426e4b232c 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1236,22 +1236,21 @@ static __be32 confirm_addr_indev(struct in_device *in_dev, __be32 dst,
/*
* Confirm that local IP address exists using wildcards:
- * - in_dev: only on this interface, 0=any interface
+ * - net: netns to check, cannot be NULL
+ * - in_dev: only on this interface, NULL=any interface
* - dst: only in the same subnet as dst, 0=any dst
* - local: address, 0=autoselect the local address
* - scope: maximum allowed scope value for the local address
*/
-__be32 inet_confirm_addr(struct in_device *in_dev,
+__be32 inet_confirm_addr(struct net *net, struct in_device *in_dev,
__be32 dst, __be32 local, int scope)
{
__be32 addr = 0;
struct net_device *dev;
- struct net *net;
- if (scope != RT_SCOPE_LINK)
+ if (in_dev != NULL)
return confirm_addr_indev(in_dev, dst, local, scope);
- net = dev_net(in_dev->dev);
rcu_read_lock();
for_each_netdev_rcu(net, dev) {
in_dev = __in_dev_get_rcu(dev);
--
1.8.4.1
^ permalink raw reply related
* [PATCH RFC] netlink: a flag requesting header w/o data in error ACK
From: Jiri Benc @ 2013-11-13 15:46 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Thomas Graf, Jamal Hadi Salim, Pablo Neira Ayuso
Currently, ACK in case of error contains a full copy of the originating
message, although many applications do not need it. This can cause lost ACKs
with large netlink messages, especially after commit c05cdb1b864f ("netlink:
allow large data transfers from user-space").
Introduce a new message flag, NLM_F_SHORT_NACK, which requests only message
header to be included in ACK regardless of the error code. This flag has no
effect if NLM_F_ACK is not set.
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
I'm not entirely happy with taking a bit in netlink message header for this.
Alternative approach would be to make this a socket option (as suggested by
David) but that would mean a socket lookup in netlink_ack for each and every
ACK, which doesn't sound nice, either. If this is preferred, I'll rework the
patch.
---
include/uapi/linux/netlink.h | 3 ++-
net/netlink/af_netlink.c | 10 +++++++---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 1a85940..d21dc2b 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -53,7 +53,8 @@ struct nlmsghdr {
#define NLM_F_MULTI 2 /* Multipart message, terminated by NLMSG_DONE */
#define NLM_F_ACK 4 /* Reply with ack, with zero or error code */
#define NLM_F_ECHO 8 /* Echo this request */
-#define NLM_F_DUMP_INTR 16 /* Dump was inconsistent due to sequence change */
+#define NLM_F_DUMP_INTR 0x10 /* Dump was inconsistent due to sequence change */
+#define NLM_F_SHORT_NACK 0x20 /* Quote only the header in ack on error */
/* Modifiers to GET request */
#define NLM_F_ROOT 0x100 /* specify tree root */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 8df7f64..7ea2a1e 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2716,13 +2716,17 @@ EXPORT_SYMBOL(__netlink_dump_start);
void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
{
+ int full_quote;
struct sk_buff *skb;
struct nlmsghdr *rep;
struct nlmsgerr *errmsg;
size_t payload = sizeof(*errmsg);
- /* error messages get the original request appened */
- if (err)
+ /* Error messages get the original request appened, unless
+ * NLM_F_SHORT_NACK is set.
+ */
+ full_quote = err && !(nlh->nlmsg_flags & NLM_F_SHORT_NACK);
+ if (full_quote)
payload += nlmsg_len(nlh);
skb = netlink_alloc_skb(in_skb->sk, nlmsg_total_size(payload),
@@ -2745,7 +2749,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
NLMSG_ERROR, payload, 0);
errmsg = nlmsg_data(rep);
errmsg->error = err;
- memcpy(&errmsg->msg, nlh, err ? nlh->nlmsg_len : sizeof(*nlh));
+ memcpy(&errmsg->msg, nlh, full_quote ? nlh->nlmsg_len : sizeof(*nlh));
netlink_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid, MSG_DONTWAIT);
}
EXPORT_SYMBOL(netlink_ack);
--
1.7.6.5
^ permalink raw reply related
* Re: [PATCH] svcrpc: remove the unnecessary evaluation
From: J. Bruce Fields @ 2013-11-13 15:56 UTC (permalink / raw)
To: Weng Meiling
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Li Zefan
In-Reply-To: <527C9160.3070808-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
On Fri, Nov 08, 2013 at 03:23:12PM +0800, Weng Meiling wrote:
>
> From: Weng Meiling <wengmeiling.weng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Huh. Looks like that was introduced with "[PATCH] knfsd: make sure
svc_process call the correct pg_authenticate for multi-service port" in
2005. OK! Applied.
--b.
>
> Signed-off-by: Weng Meiling <wengmeiling.weng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> net/sunrpc/svc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index b974571..e7fbe36 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1104,8 +1104,6 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> rqstp->rq_vers = vers = svc_getnl(argv); /* version number */
> rqstp->rq_proc = proc = svc_getnl(argv); /* procedure number */
>
> - progp = serv->sv_program;
> -
> for (progp = serv->sv_program; progp; progp = progp->pg_next)
> if (prog == progp->pg_prog)
> break;
> --
> 1.8.3.1
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Fwd: Re: [PATCH v2 2/2] x86: add prefetching to do_csum]
From: Neil Horman @ 2013-11-13 16:01 UTC (permalink / raw)
To: David Laight
Cc: Ingo Molnar, Joe Perches, netdev, Dave Jones, linux-kernel,
sebastien.dugue, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, Eric Dumazet, Peter Zijlstra
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7412@saturn3.aculab.com>
On Wed, Nov 13, 2013 at 01:32:50PM -0000, David Laight wrote:
> > > I'm not sure, whats the typical capacity for the branch predictors
> > > ability to remember code paths?
> ...
> >
> > For such simple single-target branches it goes near or over a thousand for
> > recent Intel and AMD microarchitectures. Thousands for really recent CPUs.
>
> IIRC the x86 can also correctly predict simple sequences - like a branch
> in a loop that is taken every other iteration, or only after a previous
> branch is taken.
>
> Much simpler cpus may use a much simpler strategy.
> I think one I've used (a fpga soft-core cpu) just uses the low
> bits of the instruction address to index a single bit table.
> This means that branches alias each other.
> In order to get the consistent cycle counts in order to minimise
> the worst case code path we had to disable the dynamic prediction.
>
> For the checksum code the loop branch isn't a problem.
> Tests on entry to the function might get mispredicted.
>
> So if you have conditional prefetch when the buffer is long
> then time a short buffer after a 100 long ones you'll almost
> certainly see the mispredition penalty.
>
> FWIW I remember speeding up a copy (I think) loop on a strongarm by
> adding an extra instruction to fetch a word from later in the buffer
> into a register I never otherwise used.
> (That was an unpaged system so I knew it couldn't fault.)
>
Fair enough, but the code we're looking at here is arch specific. If strongarms
benefit from different coding patterns, we can handle that in that arch. This
x86 implementation can still avoid worrying about branch predicition since its
hardware handles it well
Neil
> David
>
>
>
>
>
^ permalink raw reply
* [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Nikolay Aleksandrov @ 2013-11-13 16:07 UTC (permalink / raw)
To: netdev
Cc: davem, Nikolay Aleksandrov, Jay Vosburgh, Andy Gospodarek,
Veaceslav Falico
This patch fixes two race conditions between bond_store_updelay/downdelay
and bond_store_miimon which could lead to division by zero as miimon can
be set to 0 while either updelay/downdelay are being set and thus miss the
zero check in the beginning, the zero div happens because updelay/downdelay
are stored as new_value / bond->params.miimon. Use rtnl to synchronize with
miimon setting.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_sysfs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index bc8fd36..8f8a607 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -701,6 +701,8 @@ static ssize_t bonding_store_downdelay(struct device *d,
int new_value, ret = count;
struct bonding *bond = to_bond(d);
+ if (!rtnl_trylock())
+ return restart_syscall();
if (!(bond->params.miimon)) {
pr_err("%s: Unable to set down delay as MII monitoring is disabled\n",
bond->dev->name);
@@ -734,6 +736,7 @@ static ssize_t bonding_store_downdelay(struct device *d,
}
out:
+ rtnl_unlock();
return ret;
}
static DEVICE_ATTR(downdelay, S_IRUGO | S_IWUSR,
@@ -756,6 +759,8 @@ static ssize_t bonding_store_updelay(struct device *d,
int new_value, ret = count;
struct bonding *bond = to_bond(d);
+ if (!rtnl_trylock())
+ return restart_syscall();
if (!(bond->params.miimon)) {
pr_err("%s: Unable to set up delay as MII monitoring is disabled\n",
bond->dev->name);
@@ -789,6 +794,7 @@ static ssize_t bonding_store_updelay(struct device *d,
}
out:
+ rtnl_unlock();
return ret;
}
static DEVICE_ATTR(updelay, S_IRUGO | S_IWUSR,
--
1.8.1.4
^ permalink raw reply related
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Alan Stern @ 2013-11-13 16:20 UTC (permalink / raw)
To: David Laight
Cc: Sarah Sharp, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B740B-CgBM+Bx2aUAnGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
On Wed, 13 Nov 2013, David Laight wrote:
> > > Since you don't really want to do all the work twice, the sensible
> > > way is to add each input fragment to the ring one a time.
> > > If you need to cross a link TRB and the last MBP boundary is within
> > > the previous data TRB then you can split the previous data TRB at the
> > > MBP boundary and continue.
> > > If the previous TRB is short then you'd need to go back through the
> > > earlier TRB until you found the one that contains a TRB boundary,
> > > split it, and write a link TRB in the following slot.
> >
> > Right. You could avoid doing a lot of work twice by using two passes.
> > In the first pass, keep track of the number of bytes allotted to each
> > TRB and the MBP boundaries, without doing anything else. That way,
> > you'll know where to put a Link TRB without any need for backtracking.
> > Then in the second pass, fill in the actual contents of the TRBs.
>
> No - you really don't want to process everything twice.
What I described does not process anything twice. It calculates the
byte counts on the first pass and everything else on the second pass.
> You could remember the TRB and offset of the last MBP boundary.
Then you really _do_ end up processing the TRB's which follow the last
MBP boundary twice. Once when setting up the TD as normal, and then
again after you realize they need to be moved to a different ring
segment.
> > Unless the first TRB of the message is the first TRB of the ring
> > segment. Then you're in trouble... Hopefully, real URBs will never be
> > that badly fragmented.
>
> I suspect that ones from the network stack might be badly fragmented.
> The code needs to at least detect and error them.
I'm not so sure about this. The network stack works okay with other
host controller drivers (like ehci-hcd), which have much stricter
requirements about fragmentation. They require that the length of each
SG element except the last one must be a multiple of the maxpacket
size. For high-speed bulk transfers, that means a multiple of 512.
> > > You can split a bulk TD on a 1k boundary and the target won't know the
> > > difference.
> >
> > The target won't know the difference, but the host will. Here's an
> > example: Suppose the driver submits two URBs, each for a data-in
> > transfer of 32 KB. You split each URB up into two 16-KB TDs; let's
> > call them A, B, C, and D (where A and B make up the first URB, and C
> > and D make up the second URB).
>
> I was thinking about OUT transfers, IN ones are unlikely to be badly
> fragmented.
Maybe not for the network stack, but OUT and IN end up equally
fragmented for the storage stack.
> > Suppose you have only two ring segments, and a driver submits an URB
> > which is so fragmented that it requires more TRBs than you have room
> > for in those two segments. When do you want the interrupts to arrive?
> > Answer: At each segment crossing.
>
> You bounce the original request and fix the driver to submit URB
> with fewer fragments.
In other words, you want to limit the number of SG elements the driver
will accept.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Veaceslav Falico @ 2013-11-13 16:21 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG; +Cc: Vlad Yasevich, Linux Netdev List
In-Reply-To: <5283980D.7020508@profihost.ag>
On Wed, Nov 13, 2013 at 04:17:33PM +0100, Stefan Priebe - Profihost AG wrote:
>Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>> Hi Falico,
>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>> wrote:
>>>>> Hello,
>>>>>
>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>> never
>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>
>>>>> I've this:
>>>>>
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> /
>>>>> eth3
>>>>>
>>>>> This works fine and as expected now i want to have a vlan using the
>>>>> bonding and using a bridge.
>>>>>
>>>>> I the past i had this:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 \ vmbr1.3000
>>>>> \ ---- tap114i1
>>>>>
>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>> is bad!
>>>>
>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>> patchsets that will never hit 3.10 stable.
>>>> Also, the bonding configuration would be welcome.
>>>
>>> Debian Bonding konfiguration looks like this:
>>> auto bond1
>>> iface bond1 inet manual
>>> slaves eth2 eth3
>>> bond-mode 802.3ad
>>> bond_miimon 100
>>> bond_updelay 200
>>> bond_downdelay 0
>>>
>>> This should be miimon using lacp and not arp isn't it?
>>> Anything more needed?
>>>
>>
>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>> bond should place all of its ports into promiscuous mode. Do you see
>> the the kernel messages that say that?
>
>No it does not - i only see:
># dmesg -c|egrep "promiscuous|forward"
>[ 5.445161] device bond0 entered promiscuous mode
>[ 7.670701] device bond1 entered promiscuous mode
>[ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>[ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>[ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>[ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>
>Now adding variant 1:
># dmesg -c|egrep "promiscuous|forward"
>[ 85.919382] device tap113i0 entered promiscuous mode
>[ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>[ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>[ 86.263292] device tap113i1 entered promiscuous mode
>[ 86.314151] device vmbr1.3000 entered promiscuous mode
>[ 86.314153] device vmbr1 entered promiscuous mode
>[ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>[ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>[ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>[ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>[ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>
>Now it looks like this:
># ip a l|grep PROMISC
>13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>htb master vmbr0 state UNKNOWN qlen 500
>14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>htb master vmbr1v3000 state UNKNOWN qlen 500
eth* should get into forwarding mode cause bond0 is a port of the bridge
and should propagate its state towards its slaves. Something is wrong here.
Maybe we're looking at the wrong direction - and the promisc for the
ethernet drivers got broken?
What ethernet cards/driver do you use for eth*?
>
>Greets,
>Stefan
>
>
>Main question is - is this one correct:
Both are correct. Here's my setup (sorry for stretching):
+---------------+ +------------+ +-------------+ +---------+ +------+
| bond1 | | | | bridge0 | | | | |
| 192.168.2.1 | master | bridge0.15 | neighbour | 192.168.3.1 | master | bond0 | master | eth2 |
| | --------> | | ------------ | 192.168.4.1 | --------> | | --------> | |
+---------------+ +------------+ +-------------+ +---------+ +------+
|
| master
v
+---------------+ +---------+
| dummy0 | | eth0 |
+---------------+ +---------+
(disregard that dummy0).
All 192.168.X.1 ips are pingable (via the correct vlans) on both net-next and stable 3.10.19.
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 \ vmbr1.3000
>>>>> \ ---- tap114i1
>
><= does not work at all
>
>or this one?:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>> \ ---- tap114i1
>
><= works if i manually put eth2 and eth3 into promiscous mode.
>
>> -vlad
>>
>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>> promisc mode.
>>>
>>> Stefan
>>>
>>>>> I also tried this one without success:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>> \ ---- tap114i1
>>>>>
>>>>>
>>>>>
>>>>> Greets,
>>>>> Stefan
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Alan Stern @ 2013-11-13 16:27 UTC (permalink / raw)
To: David Laight
Cc: Sarah Sharp, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B740C-CgBM+Bx2aUAnGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
On Wed, 13 Nov 2013, David Laight wrote:
> > > that doesn't matter; we don't get an interrupt when a ring segment is
> > > crossed. Instead we set the interrupt-on-completion flag on the last
> > > TRB of the TD, not on any earlier fragment or link TRB.
> >
> > That's because you don't worry about handling URBs which require too
> > many TRBs (i.e., more than are available). You just add more ring
> > segments. Instead, you could re-use segments on the fly.
> >
> > For example, suppose you have only two ring segments and you get an URB
> > which requires enough TRBs to fill up four segments. You could fill in
> > the first two segments worth, and get an interrupt when the controller
> > traverses the Link TRB between them. At that point you store the third
> > set of TRBs in the first segment, which is now vacant. Similarly, when
> > the second Link TRB is traversed, you fill in the fourth set of TRBs.
>
> That isn't going to work.
Queuing URBs and processing them in pieces was _your_ suggestion. I
merely repeated it to Sarah and filled in some detail.
> It might work for very long TD, but for very fragmented ones the interrupt
> rate would be stupid.
Not if the individual SG elements are reasonably large -- say at least
512 bytes. They have to be that big if they are to work with ehci-hcd,
so why not also for xhci-hcd?
> In any case you'd definitely need enough ring
> segments for the TRB that describe a single 'max packet size' block.
That would be two TRBs (for maxpacket = 1024), meaning you'd need at
least one ring segment. That's not much of a restriction.
> > > Finally, it's interesting to note that the USB mass storage driver is
> > > using scatter gather lists just fine without the driver following the TD
> > > fragment rules. Or at least no one has reported any issues. I wonder
> > > why it works?
> >
> > I'd guess this is because the hardware is actually a lot more flexible
> > than the "No Link TRBs in the middle of a TD fragment" rule.
>
> With the hardware I have (Intel i7 Sandy bridge) Link TRB cannot
> be placed at arbitrary boundaries.
> I don't know whether the actual restriction is only to packet boundaries.
That's what I had in mind by "more flexible" above.
> I don't have a USB3 monitor and it would also require a more contrived
> test than I've been doing.
You wouldn't need a monitor to check it, but you would need some
careful tests.
> > The whole idea of TD fragments makes no sense to begin with. What
> > point is there in grouping packets into MaxBurst-sized collections?
>
> It probably saved a few logic gates somewhere, either that or it is
> a bug in some hardware implementation that got documented in the spec
> instead of being fixed :-)
I can believe the guess about it reflecting a bug. Particularly since
it is so badly written.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Nikolay Aleksandrov @ 2013-11-13 16:34 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev
Cc: davem, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
In-Reply-To: <1384358866-15157-1-git-send-email-nikolay@redhat.com>
On 11/13/2013 05:07 PM, Nikolay Aleksandrov wrote:
> This patch fixes two race conditions between bond_store_updelay/downdelay
> and bond_store_miimon which could lead to division by zero as miimon can
> be set to 0 while either updelay/downdelay are being set and thus miss the
> zero check in the beginning, the zero div happens because updelay/downdelay
> are stored as new_value / bond->params.miimon. Use rtnl to synchronize with
> miimon setting.
>
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
A little addition: these can also race with anything that sets miimon to 0,
currently beside store_miimon only store_arp_interval does that. But that is
also fixed because store_arp_interval uses rtnl.
Cheers,
Nik
^ permalink raw reply
* Re: [PATCH net-next 4/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Ronen Hod @ 2013-11-13 16:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Michael Dalton, Michael S. Tsirkin, netdev, Daniel Borkmann,
virtualization, Eric Dumazet, David S. Miller
In-Reply-To: <1384352348.28458.102.camel@edumazet-glaptop2.roam.corp.google.com>
On 11/13/2013 04:19 PM, Eric Dumazet wrote:
> On Wed, 2013-11-13 at 10:47 +0200, Ronen Hod wrote:
>
>> I looked at how ewma works, and although it is computationally efficient,
>> and it does what it is supposed to do, initially (at the first samples) it is strongly
>> biased towards the value that was added at the first ewma_add.
>> I suggest that you print the values of ewma_add() and ewma_read(). If you are
>> happy with the results, then ignore my comments. If you are not, then I can
>> provide a version that does better for the first samples.
>> Unfortunately, it will be slightly less efficient.
> Value is clamped by (GOOD_PACKET_LEN, PAGE_SIZE - hdr_len)
>
> So initial value is conservative and not really used.
Hi Eric,
This initial value, that you do not really want to use, will slowly fade, but it
will still pretty much dominate the returned value for the first RECEIVE_AVG_WEIGHT(==64)
samples or so (most ewma implementations suffer from this bug).
Naturally, it doesn't matter much if you just keep it running forever.
However, if you will want to restart the learning process more often, which might make
sense upon changes, then the auto-tuning will be very sub-optimal.
Ronen.
> Thanks
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Stefan Priebe - Profihost AG @ 2013-11-13 16:43 UTC (permalink / raw)
To: Veaceslav Falico; +Cc: Vlad Yasevich, Linux Netdev List
In-Reply-To: <20131113162136.GP19702@redhat.com>
> Am 13.11.2013 um 17:21 schrieb Veaceslav Falico <vfalico@redhat.com>:
>
>> On Wed, Nov 13, 2013 at 04:17:33PM +0100, Stefan Priebe - Profihost AG wrote:
>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>> Hi Falico,
>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>> never
>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>
>>>>>> I've this:
>>>>>>
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> /
>>>>>> eth3
>>>>>>
>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>> bonding and using a bridge.
>>>>>>
>>>>>> I the past i had this:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>>> is bad!
>>>>>
>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>> patchsets that will never hit 3.10 stable.
>>>>> Also, the bonding configuration would be welcome.
>>>>
>>>> Debian Bonding konfiguration looks like this:
>>>> auto bond1
>>>> iface bond1 inet manual
>>>> slaves eth2 eth3
>>>> bond-mode 802.3ad
>>>> bond_miimon 100
>>>> bond_updelay 200
>>>> bond_downdelay 0
>>>>
>>>> This should be miimon using lacp and not arp isn't it?
>>>> Anything more needed?
>>>
>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>> bond should place all of its ports into promiscuous mode. Do you see
>>> the the kernel messages that say that?
>>
>> No it does not - i only see:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 5.445161] device bond0 entered promiscuous mode
>> [ 7.670701] device bond1 entered promiscuous mode
>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>
>> Now adding variant 1:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 85.919382] device tap113i0 entered promiscuous mode
>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 86.263292] device tap113i1 entered promiscuous mode
>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>> [ 86.314153] device vmbr1 entered promiscuous mode
>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>
>> Now it looks like this:
>> # ip a l|grep PROMISC
>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr0 state UNKNOWN qlen 500
>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr1v3000 state UNKNOWN qlen 500
>
> eth* should get into forwarding mode cause bond0 is a port of the bridge
> and should propagate its state towards its slaves. Something is wrong here.
>
> Maybe we're looking at the wrong direction - and the promisc for the
> ethernet drivers got broken?
>
> What ethernet cards/driver do you use for eth*?
intel igb
>
>>
>> Greets,
>> Stefan
>>
>>
>> Main question is - is this one correct:
>
> Both are correct. Here's my setup (sorry for stretching):
>
> +---------------+ +------------+ +-------------+ +---------+ +------+
> | bond1 | | | | bridge0 | | | | |
> | 192.168.2.1 | master | bridge0.15 | neighbour | 192.168.3.1 | master | bond0 | master | eth2 |
> | | --------> | | ------------ | 192.168.4.1 | --------> | | --------> | |
> +---------------+ +------------+ +-------------+ +---------+ +------+
> |
> | master
> v
> +---------------+ +---------+
> | dummy0 | | eth0 |
> +---------------+ +---------+
>
> (disregard that dummy0).
>
> All 192.168.X.1 ips are pingable (via the correct vlans) on both net-next and stable 3.10.19.
>
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>
>> <= does not work at all
>>
>> or this one?:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>
>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>
>>> -vlad
>>>
>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>> promisc mode.
>>>>
>>>> Stefan
>>>>
>>>>>> I also tried this one without success:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>>
>>>>>>
>>>>>> Greets,
>>>>>> Stefan
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Vlad Yasevich @ 2013-11-13 16:44 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG, Veaceslav Falico; +Cc: Linux Netdev List
In-Reply-To: <5283980D.7020508@profihost.ag>
On 11/13/2013 10:17 AM, Stefan Priebe - Profihost AG wrote:
> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>> Hi Falico,
>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>> wrote:
>>>>> Hello,
>>>>>
>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>> never
>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>
>>>>> I've this:
>>>>>
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> /
>>>>> eth3
>>>>>
>>>>> This works fine and as expected now i want to have a vlan using the
>>>>> bonding and using a bridge.
>>>>>
>>>>> I the past i had this:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 \ vmbr1.3000
>>>>> \ ---- tap114i1
>>>>>
>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>> is bad!
>>>>
>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>> patchsets that will never hit 3.10 stable.
>>>> Also, the bonding configuration would be welcome.
>>>
>>> Debian Bonding konfiguration looks like this:
>>> auto bond1
>>> iface bond1 inet manual
>>> slaves eth2 eth3
>>> bond-mode 802.3ad
>>> bond_miimon 100
>>> bond_updelay 200
>>> bond_downdelay 0
>>>
>>> This should be miimon using lacp and not arp isn't it?
>>> Anything more needed?
>>>
>>
>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>> bond should place all of its ports into promiscuous mode. Do you see
>> the the kernel messages that say that?
>
> No it does not - i only see:
> # dmesg -c|egrep "promiscuous|forward"
> [ 5.445161] device bond0 entered promiscuous mode
> [ 7.670701] device bond1 entered promiscuous mode
> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>
So this can happen if bond interfaces are down at the time of joining
the bridge. The promisc flag propagation to lower interfaces only
happens when those interfaces are up. So if for whatever reason,
the lower interfaces happen to be down during the crating of this
hierarchy, the promiscuity flag may not get set on the lower level port
device an we may end up in this situation.
-vlad
> Now adding variant 1:
> # dmesg -c|egrep "promiscuous|forward"
> [ 85.919382] device tap113i0 entered promiscuous mode
> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
> [ 86.263292] device tap113i1 entered promiscuous mode
> [ 86.314151] device vmbr1.3000 entered promiscuous mode
> [ 86.314153] device vmbr1 entered promiscuous mode
> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>
> Now it looks like this:
> # ip a l|grep PROMISC
> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
> htb master vmbr0 state UNKNOWN qlen 500
> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
> htb master vmbr1v3000 state UNKNOWN qlen 500
>
> Greets,
> Stefan
>
>
> Main question is - is this one correct:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 \ vmbr1.3000
>>>>> \ ---- tap114i1
>
> <= does not work at all
>
> or this one?:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>> \ ---- tap114i1
>
> <= works if i manually put eth2 and eth3 into promiscous mode.
>
>> -vlad
>>
>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>> promisc mode.
>>>
>>> Stefan
>>>
>>>>> I also tried this one without success:
>>>>> eth2
>>>>> \
>>>>> -- bond1 -- vmbr1
>>>>> / \
>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>> \ ---- tap114i1
>>>>>
>>>>>
>>>>>
>>>>> Greets,
>>>>> Stefan
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-13 16:58 UTC (permalink / raw)
To: Alan Stern
Cc: Sarah Sharp, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <Pine.LNX.4.44L0.1311131113090.1424-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
> -----Original Message-----
> From: Alan Stern [mailto:stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org]
> Sent: 13 November 2013 16:21
> To: David Laight
> Cc: Sarah Sharp; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
>
> On Wed, 13 Nov 2013, David Laight wrote:
>
> > > > Since you don't really want to do all the work twice, the sensible
> > > > way is to add each input fragment to the ring one a time.
> > > > If you need to cross a link TRB and the last MBP boundary is within
> > > > the previous data TRB then you can split the previous data TRB at the
> > > > MBP boundary and continue.
> > > > If the previous TRB is short then you'd need to go back through the
> > > > earlier TRB until you found the one that contains a TRB boundary,
> > > > split it, and write a link TRB in the following slot.
> > >
> > > Right. You could avoid doing a lot of work twice by using two passes.
> > > In the first pass, keep track of the number of bytes allotted to each
> > > TRB and the MBP boundaries, without doing anything else. That way,
> > > you'll know where to put a Link TRB without any need for backtracking.
> > > Then in the second pass, fill in the actual contents of the TRBs.
> >
> > No - you really don't want to process everything twice.
>
> What I described does not process anything twice. It calculates the
> byte counts on the first pass and everything else on the second pass.
>
> > You could remember the TRB and offset of the last MBP boundary.
>
> Then you really _do_ end up processing the TRB's which follow the last
> MBP boundary twice. Once when setting up the TD as normal, and then
> again after you realize they need to be moved to a different ring
> segment.
That would only be a few of the TRBs, not all of them.
The cheap option is to work out an upper bound for the number of TRB
and the write a new LINK TRB if there isn't enough space.
That does limit the maximum number of TRB in a URB to half the total
ring (less if it has more than 2 fragments).
> > > Unless the first TRB of the message is the first TRB of the ring
> > > segment. Then you're in trouble... Hopefully, real URBs will never be
> > > that badly fragmented.
> >
> > I suspect that ones from the network stack might be badly fragmented.
> > The code needs to at least detect and error them.
>
> I'm not so sure about this. The network stack works okay with other
> host controller drivers (like ehci-hcd), which have much stricter
> requirements about fragmentation. They require that the length of each
> SG element except the last one must be a multiple of the maxpacket
> size. For high-speed bulk transfers, that means a multiple of 512.
Yes, for other host controllers the network cards linearise the skb.
it is only for xhci where where the buffer fragments from the skb
can be given to the usb controller.
This is probably also only enabled (at the moment) for the ASIX Ge card
- which can also do TCP transmit segmentation offload (TSO).
It is effectively a necessity for TSO since it would otherwise
require 64k blocks of contiguous memory (or some horrid code that
involves a misaligned copy).
The SG code in usbnet was only added for 3.12.
> > > > You can split a bulk TD on a 1k boundary and the target won't know the
> > > > difference.
> > >
> > > The target won't know the difference, but the host will. Here's an
> > > example: Suppose the driver submits two URBs, each for a data-in
> > > transfer of 32 KB. You split each URB up into two 16-KB TDs; let's
> > > call them A, B, C, and D (where A and B make up the first URB, and C
> > > and D make up the second URB).
> >
> > I was thinking about OUT transfers, IN ones are unlikely to be badly
> > fragmented.
>
> Maybe not for the network stack, but OUT and IN end up equally
> fragmented for the storage stack.
But the minimum fragment size is (probably) 4k.
For the network stack an OUT transfer might have a lot (and I mean lots)
of fragments (there may be constraints, and linearising the skb is a option).
> > > Suppose you have only two ring segments, and a driver submits an URB
> > > which is so fragmented that it requires more TRBs than you have room
> > > for in those two segments. When do you want the interrupts to arrive?
> > > Answer: At each segment crossing.
> >
> > You bounce the original request and fix the driver to submit URB
> > with fewer fragments.
>
> In other words, you want to limit the number of SG elements the driver
> will accept.
Yes, passing the buck to the higher layer.
If the ring was a single segment of 256 slots you'd have to limit the
number of TRB to 128. If the alignment is horrid this could be under 64
fragments (because of the 64k boundary restriction).
What I don't know is whether the code to increase the ring size was added
because a single URB contained too many fragments, or because a lot
of URB were submitted at once - I don't remember anything in the commit
messages about why.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Eric Dumazet @ 2013-11-13 17:03 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
In-Reply-To: <1384358866-15157-1-git-send-email-nikolay@redhat.com>
On Wed, 2013-11-13 at 17:07 +0100, Nikolay Aleksandrov wrote:
> This patch fixes two race conditions between bond_store_updelay/downdelay
> and bond_store_miimon which could lead to division by zero as miimon can
> be set to 0 while either updelay/downdelay are being set and thus miss the
> zero check in the beginning, the zero div happens because updelay/downdelay
> are stored as new_value / bond->params.miimon. Use rtnl to synchronize with
> miimon setting.
It seems a bit heavy duty to take rtnl for this.
Using ACCESS_ONCE() in bonding_store_updelay()/bonding_store_downdelay()
should be enough ?
int miimon = ACCESS_ONCE(bond->params.miimon);
^ permalink raw reply
* Re: oom-kill && frozen()
From: Oleg Nesterov @ 2013-11-13 17:07 UTC (permalink / raw)
To: Tejun Heo
Cc: Peter Zijlstra, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113032053.GA19394@mtj.dyndns.org>
On 11/13, Tejun Heo wrote:
>
> Hello,
>
> On Tue, Nov 12, 2013 at 05:56:43PM +0100, Oleg Nesterov wrote:
> > On 11/12, Oleg Nesterov wrote:
> > > I am also wondering if it makes any sense to turn PF_FROZEN into
> > > TASK_FROZEN, something like (incomplete, probably racy) patch below.
> > > Note that it actually adds the new state, not the the qualifier.
> >
> > As for the current usage of PF_FROZEN... David, it seems that
> > oom_scan_process_thread()->__thaw_task() is dead? Probably this
> > was fine before, when __thaw_task() cleared the "need to freeze"
> > condition, iirc it was PF_FROZEN.
> >
> > But today __thaw_task() can't help, no? the task will simply
> > schedule() in D state again.
>
> Yeah, it'll have to be actively excluded using e.g. PF_FREEZER_SKIP,
> which, BTW, can usually only be manipulated by the task itself.
Oh, yes, yes, yes, I agree. PF_FREEZER_SKIP and the growing number of
freezable_schedule() makes this all more confusing.
In fact I was think about something like
1. Add the new __TASK_FREEZABLE qualifier
2. Turn freezable_schedule() into
void freezable_schedule(void)
{
spin_lock_irq(¤t->pi_lock);
if (current->state)
current->state |= __TASK_FREEZABLE
spin_unlock_irq(¤t->pi_lock);
schedule();
try_to_freeze();
}
3. Kill PF_FREEZER_SKIP/freezer_do_not_count/count/should_skip
4. Change freeze_task() and fake_signal_wake_up()
- wake_up_state(p, TASK_INTERRUPTIBLE);
+ wake_up_state(p, TASK_INTERRUPTIBLE | __TASK_FREEZABLE);
Unfortunately, this can only work if the caller can tolerate the
false wakeup. We can even fix wait_for_vfork_done(), but say
ptrace_stop() can't work this way.
And even if we can make this work, the very fact that freezable_schedule()
does schedule() twice does not look right.
_Perhaps_ we can do something like "selective wakeup"? IOW, ignoring the
races/details,
1. Add __TASK_FROZEN qualifier _and_ state
2. Change frozen(),
static inline bool frozen(struct task_struct *p)
{
return p->state & __TASK_FROZEN;
}
2. Change freezable_schedule(),
void freezable_schedule(void)
{
spin_lock_irq(¤t->pi_lock);
if (current->state)
current->state |= __TASK_FROZEN;
spin_unlock_irq(¤t->pi_lock);
schedule();
}
3. Change __refrigerator() to use saved_state | __TASK_FROZEN
too.
4. Finally, change try_to_wake_up() path to do
- p->state = TASK_WAKING;
+ p->state &= ~state;
+ if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
+ return;
+ else
+ p->state = TASK_WAKING;
IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
try_to_wake_up(__TASK_FROZEN) to wake up.
5. Kill PF_FREEZER_SKIP / etc.
Unfortunately, 4. is obviously needs more changes, although at first glance
nothing really nontrivial... we need a common helper for try_to_wake_up()
and ttwu_remote() which checks/changes ->state and we need to avoid "stat"
if we do not actually wake up.
Hmm. and this all makes me think that at least s/PF_FROZEN/TASK_FROZEN/ as
a first step actually makes some sense... Note the "qualifier _and_ state"
above.
Tejun, Peter, do you think this makes any sense? I am just curious, but
"selective wakeup" looks potentially useful.
And what about oom_scan_process_thread() ? should we simply kill this
dead frozen/__thaw_task code or should we change freezing() to respect
TIF_MEMDIE?
Oleg.
^ permalink raw reply
* Re: [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Nikolay Aleksandrov @ 2013-11-13 17:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, davem, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
In-Reply-To: <1384362214.28458.117.camel@edumazet-glaptop2.roam.corp.google.com>
On 11/13/2013 06:03 PM, Eric Dumazet wrote:
> On Wed, 2013-11-13 at 17:07 +0100, Nikolay Aleksandrov wrote:
>> This patch fixes two race conditions between bond_store_updelay/downdelay
>> and bond_store_miimon which could lead to division by zero as miimon can
>> be set to 0 while either updelay/downdelay are being set and thus miss the
>> zero check in the beginning, the zero div happens because updelay/downdelay
>> are stored as new_value / bond->params.miimon. Use rtnl to synchronize with
>> miimon setting.
>
> It seems a bit heavy duty to take rtnl for this.
>
> Using ACCESS_ONCE() in bonding_store_updelay()/bonding_store_downdelay()
> should be enough ?
>
> int miimon = ACCESS_ONCE(bond->params.miimon);
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Eric,
I thought about this version too, but downdelay/updelay can be changed in other
places (e.g., store_miimon) and the resulting downdelay/updelay value might not
be the right one.
Correct me if I'm wrong, but this is what I have in mind (miimon = 100, updelay
= 200):
set miimon to 300 and concurrently set updelay to 400, we might endup leaving
updelay to 400 because the old value of miimon is used in the calculation in
store_updelay even though when changing miimon updelay/downdelay get adjusted,
they might get adjusted by store_updelay/downdelay to a wrong value afterwards.
Nik
^ permalink raw reply
* Re: [PATCH net-next 4/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Eric Dumazet @ 2013-11-13 17:18 UTC (permalink / raw)
To: Ronen Hod
Cc: Michael Dalton, Michael S. Tsirkin, netdev, Daniel Borkmann,
virtualization, Eric Dumazet, David S. Miller
In-Reply-To: <5283AC15.8090706@redhat.com>
On Wed, 2013-11-13 at 18:43 +0200, Ronen Hod wrote:
>
> This initial value, that you do not really want to use, will slowly fade, but it
> will still pretty much dominate the returned value for the first RECEIVE_AVG_WEIGHT(==64)
> samples or so (most ewma implementations suffer from this bug).
> Naturally, it doesn't matter much if you just keep it running forever.
> However, if you will want to restart the learning process more often, which might make
> sense upon changes, then the auto-tuning will be very sub-optimal.
Note that we fill a ring buffer at open time (try_fill_recv()),
all these buffers will be of the minimal size.
By the time we have refilled the ring buffer, EWMA value will be
GOOD_PACKET_LEN.
These sizes are a hint, clamped between 1500 and PAGE_SIZE.
We do not care of very first allocated buffers, they are good enough.
We only care of the million of following allocations.
Also note the EWMA is per queue, not global to the device.
Of course, there is no 'one size' perfect for all usages.
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Vlad Yasevich @ 2013-11-13 17:21 UTC (permalink / raw)
To: Veaceslav Falico, Stefan Priebe - Profihost AG; +Cc: Linux Netdev List
In-Reply-To: <20131113162136.GP19702@redhat.com>
On 11/13/2013 11:21 AM, Veaceslav Falico wrote:
> On Wed, Nov 13, 2013 at 04:17:33PM +0100, Stefan Priebe - Profihost AG
> wrote:
>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>> Hi Falico,
>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>> never
>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>
>>>>>> I've this:
>>>>>>
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> /
>>>>>> eth3
>>>>>>
>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>> bonding and using a bridge.
>>>>>>
>>>>>> I the past i had this:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-(
>>>>>> this
>>>>>> is bad!
>>>>>
>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>> patchsets that will never hit 3.10 stable.
>>>>> Also, the bonding configuration would be welcome.
>>>>
>>>> Debian Bonding konfiguration looks like this:
>>>> auto bond1
>>>> iface bond1 inet manual
>>>> slaves eth2 eth3
>>>> bond-mode 802.3ad
>>>> bond_miimon 100
>>>> bond_updelay 200
>>>> bond_downdelay 0
>>>>
>>>> This should be miimon using lacp and not arp isn't it?
>>>> Anything more needed?
>>>>
>>>
>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>> bond should place all of its ports into promiscuous mode. Do you see
>>> the the kernel messages that say that?
>>
>> No it does not - i only see:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 5.445161] device bond0 entered promiscuous mode
>> [ 7.670701] device bond1 entered promiscuous mode
>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>
>> Now adding variant 1:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 85.919382] device tap113i0 entered promiscuous mode
>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 86.263292] device tap113i1 entered promiscuous mode
>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>> [ 86.314153] device vmbr1 entered promiscuous mode
>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>
>> Now it looks like this:
>> # ip a l|grep PROMISC
>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr0 state UNKNOWN qlen 500
>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr1v3000 state UNKNOWN qlen 500
>
> eth* should get into forwarding mode cause bond0 is a port of the bridge
> and should propagate its state towards its slaves. Something is wrong here.
>
> Maybe we're looking at the wrong direction - and the promisc for the
> ethernet drivers got broken?
I was able to duplicate Stefans results only when I turn off the link to
the underlying devices when building the bridge. When the link is off,
the rx_flags do not propagate down to the lower level devices.
What about something like this (completely untested)
diff --git a/drivers/net/bonding/bond_main.c
b/drivers/net/bonding/bond_main.c
index 4dd5ee2..3051744 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2863,6 +2863,17 @@ static int bond_slave_netdev_event(unsigned long
event,
bond_release(bond_dev, slave_dev);
break;
case NETDEV_UP:
+ /* If the bond was set to primisc, but slave has not due to
+ * slave being down when the command was issued, sync the
+ * state when the slave comes up.
+ */
+ if (bond_dev->flags & IFF_PROMISC &&
!slave_dev->promiscuity) {
+ if (!USES_PRIMARY(bond))
+ dev_set_promiscuity(slave_dev, 1);
+ else if (slave == bond->curr_active_slave)
+ dev_set_promiscuity(slave_dev, 1);
+ }
+
case NETDEV_CHANGE:
old_speed = slave->speed;
old_duplex = slave->duplex;
-vlad
>
> What ethernet cards/driver do you use for eth*?
>
>>
>> Greets,
>> Stefan
>>
>>
>> Main question is - is this one correct:
>
> Both are correct. Here's my setup (sorry for stretching):
>
> +---------------+ +------------+
> +-------------+ +---------+ +------+
> | bond1 | | | | bridge0
> | | | | |
> | 192.168.2.1 | master | bridge0.15 | neighbour | 192.168.3.1 |
> master | bond0 | master | eth2 |
> | | --------> | | ------------ | 192.168.4.1 |
> --------> | | --------> | |
> +---------------+ +------------+
> +-------------+ +---------+ +------+
> |
> | master
> v
> +---------------+
> +---------+
> | dummy0
> | |
> eth0 |
> +---------------+
> +---------+
>
> (disregard that dummy0).
>
> All 192.168.X.1 ips are pingable (via the correct vlans) on both
> net-next and stable 3.10.19.
>
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>
>> <= does not work at all
>>
>> or this one?:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>
>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>
>>> -vlad
>>>
>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben
>>>> not
>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>> promisc mode.
>>>>
>>>> Stefan
>>>>
>>>>>> I also tried this one without success:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>>
>>>>>>
>>>>>> Greets,
>>>>>> Stefan
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
^ permalink raw reply related
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Stefan Priebe - Profihost AG @ 2013-11-13 17:22 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: Veaceslav Falico, Linux Netdev List
In-Reply-To: <5283AC86.9060601@gmail.com>
> Am 13.11.2013 um 17:44 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>
>> On 11/13/2013 10:17 AM, Stefan Priebe - Profihost AG wrote:
>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>> Hi Falico,
>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>> wrote:
>>>>>> Hello,
>>>>>>
>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>> never
>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>
>>>>>> I've this:
>>>>>>
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> /
>>>>>> eth3
>>>>>>
>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>> bonding and using a bridge.
>>>>>>
>>>>>> I the past i had this:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>>> is bad!
>>>>>
>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>> patchsets that will never hit 3.10 stable.
>>>>> Also, the bonding configuration would be welcome.
>>>>
>>>> Debian Bonding konfiguration looks like this:
>>>> auto bond1
>>>> iface bond1 inet manual
>>>> slaves eth2 eth3
>>>> bond-mode 802.3ad
>>>> bond_miimon 100
>>>> bond_updelay 200
>>>> bond_downdelay 0
>>>>
>>>> This should be miimon using lacp and not arp isn't it?
>>>> Anything more needed?
>>>
>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>> bond should place all of its ports into promiscuous mode. Do you see
>>> the the kernel messages that say that?
>>
>> No it does not - i only see:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 5.445161] device bond0 entered promiscuous mode
>> [ 7.670701] device bond1 entered promiscuous mode
>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>
> So this can happen if bond interfaces are down at the time of joining
> the bridge.
About which of my tried configuration do you speak? The bond itself is always up it's the internet connection. Or do you mean the vlan bond?
> The promisc flag propagation to lower interfaces only happens when those interfaces are up. So if for whatever reason,
> the lower interfaces happen to be down during the crating of this
> hierarchy, the promiscuity flag may not get set on the lower level port
> device an we may end up in this situation.
>
> -vlad
>
>> Now adding variant 1:
>> # dmesg -c|egrep "promiscuous|forward"
>> [ 85.919382] device tap113i0 entered promiscuous mode
>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>> [ 86.263292] device tap113i1 entered promiscuous mode
>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>> [ 86.314153] device vmbr1 entered promiscuous mode
>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>
>> Now it looks like this:
>> # ip a l|grep PROMISC
>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr0 state UNKNOWN qlen 500
>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>> htb master vmbr1v3000 state UNKNOWN qlen 500
>>
>> Greets,
>> Stefan
>>
>>
>> Main question is - is this one correct:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 \ vmbr1.3000
>>>>>> \ ---- tap114i1
>>
>> <= does not work at all
>>
>> or this one?:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>
>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>
>>> -vlad
>>>
>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>> promisc mode.
>>>>
>>>> Stefan
>>>>
>>>>>> I also tried this one without success:
>>>>>> eth2
>>>>>> \
>>>>>> -- bond1 -- vmbr1
>>>>>> / \
>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>> \ ---- tap114i1
>>>>>>
>>>>>>
>>>>>>
>>>>>> Greets,
>>>>>> Stefan
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Vlad Yasevich @ 2013-11-13 17:37 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG; +Cc: Veaceslav Falico, Linux Netdev List
In-Reply-To: <BD2E55EF-4776-4CB3-9674-CDFB48B74465@profihost.ag>
On 11/13/2013 12:22 PM, Stefan Priebe - Profihost AG wrote:
>
>> Am 13.11.2013 um 17:44 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>>
>>> On 11/13/2013 10:17 AM, Stefan Priebe - Profihost AG wrote:
>>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>>> Hi Falico,
>>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>>> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>>> never
>>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>>
>>>>>>> I've this:
>>>>>>>
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> /
>>>>>>> eth3
>>>>>>>
>>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>>> bonding and using a bridge.
>>>>>>>
>>>>>>> I the past i had this:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 \ vmbr1.3000
>>>>>>> \ ---- tap114i1
>>>>>>>
>>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>>>> is bad!
>>>>>>
>>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>>> patchsets that will never hit 3.10 stable.
>>>>>> Also, the bonding configuration would be welcome.
>>>>>
>>>>> Debian Bonding konfiguration looks like this:
>>>>> auto bond1
>>>>> iface bond1 inet manual
>>>>> slaves eth2 eth3
>>>>> bond-mode 802.3ad
>>>>> bond_miimon 100
>>>>> bond_updelay 200
>>>>> bond_downdelay 0
>>>>>
>>>>> This should be miimon using lacp and not arp isn't it?
>>>>> Anything more needed?
>>>>
>>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>>> bond should place all of its ports into promiscuous mode. Do you see
>>>> the the kernel messages that say that?
>>>
>>> No it does not - i only see:
>>> # dmesg -c|egrep "promiscuous|forward"
>>> [ 5.445161] device bond0 entered promiscuous mode
>>> [ 7.670701] device bond1 entered promiscuous mode
>>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>
>> So this can happen if bond interfaces are down at the time of joining
>> the bridge.
>
> About which of my tried configuration do you speak? The bond itself is always up it's the internet connection. Or do you mean the vlan bond?
>
The issue is in the formation for vmbr0 and vmbr1. When bond0 and
bond1 enter promisc mode as part of becoming a bridge port, they should
put some subset of their ports into promisc mode as well (in 802.3ad
mode it should happen on all bond slaves).
What I see in my config is something like:
[ 2640.725700] device bond0 entered promiscuous mode
[ 2640.726623] device eth0 entered promiscuous mode
[ 2640.727655] br0: port 1(bond0) entered forwarding state
[ 2640.728718] br0: port 1(bond0) entered forwarding state
This is my quick configuration:
br0 ----> bond0 -------> eth0
I wanted to make sure in this case eth0 is properly configured in
promisc mode.
If eth0 is somehow DOWN, when bond0 is added to the bridge, then it
will never get promisc mode set (at least in my quick test).
I am not sure if this what's happening in your case, but that's
the only explanation I can come up with for the above log you provided.
-vlad
>
>> The promisc flag propagation to lower interfaces only happens when those interfaces are up. So if for whatever reason,
>> the lower interfaces happen to be down during the crating of this
>> hierarchy, the promiscuity flag may not get set on the lower level port
>> device an we may end up in this situation.
>>
>> -vlad
>>
>>> Now adding variant 1:
>>> # dmesg -c|egrep "promiscuous|forward"
>>> [ 85.919382] device tap113i0 entered promiscuous mode
>>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>>> [ 86.263292] device tap113i1 entered promiscuous mode
>>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>>> [ 86.314153] device vmbr1 entered promiscuous mode
>>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>
>>> Now it looks like this:
>>> # ip a l|grep PROMISC
>>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>> htb master vmbr0 state UNKNOWN qlen 500
>>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>> htb master vmbr1v3000 state UNKNOWN qlen 500
>>>
>>> Greets,
>>> Stefan
>>>
>>>
>>> Main question is - is this one correct:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 \ vmbr1.3000
>>>>>>> \ ---- tap114i1
>>>
>>> <= does not work at all
>>>
>>> or this one?:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>> \ ---- tap114i1
>>>
>>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>>
>>>> -vlad
>>>>
>>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>>> promisc mode.
>>>>>
>>>>> Stefan
>>>>>
>>>>>>> I also tried this one without success:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>> \ ---- tap114i1
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Greets,
>>>>>>> Stefan
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
* Re: [PATCH net] bonding: fix two race conditions in bond_store_updelay/downdelay
From: Eric Dumazet @ 2013-11-13 17:38 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
In-Reply-To: <5283B19A.9050302@redhat.com>
On Wed, 2013-11-13 at 18:06 +0100, Nikolay Aleksandrov wrote:
> I thought about this version too, but downdelay/updelay can be changed in other
> places (e.g., store_miimon) and the resulting downdelay/updelay value might not
> be the right one.
> Correct me if I'm wrong, but this is what I have in mind (miimon = 100, updelay
> = 200):
> set miimon to 300 and concurrently set updelay to 400, we might endup leaving
> updelay to 400 because the old value of miimon is used in the calculation in
> store_updelay even though when changing miimon updelay/downdelay get adjusted,
> they might get adjusted by store_updelay/downdelay to a wrong value afterwards.
OK then ;)
^ permalink raw reply
* Re: [PATCH net-next 1/4] virtio-net: mergeable buffer size should include virtio-net header
From: Michael S. Tsirkin @ 2013-11-13 17:39 UTC (permalink / raw)
To: Michael Dalton
Cc: netdev, Daniel Borkmann, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <1384294885-6444-1-git-send-email-mwdalton@google.com>
On Tue, Nov 12, 2013 at 02:21:22PM -0800, Michael Dalton wrote:
> Commit 2613af0ed18a ("virtio_net: migrate mergeable rx buffers to page
> frag allocators") changed the mergeable receive buffer size from PAGE_SIZE
> to MTU-size. However, the merge buffer size does not take into account the
> size of the virtio-net header. Consequently, packets that are MTU-size
> will take two buffers intead of one (to store the virtio-net header),
> substantially decreasing the throughput of MTU-size traffic due to TCP
> window / SKB truesize effects.
>
> This commit changes the mergeable buffer size to include the virtio-net
> header. The buffer size is cacheline-aligned because skb_page_frag_refill
> will not automatically align the requested size.
>
> Benchmarks taken from an average of 5 netperf 30-second TCP_STREAM runs
> between two QEMU VMs on a single physical machine. Each VM has two VCPUs and
> vhost enabled. All VMs and vhost threads run in a single 4 CPU cgroup
> cpuset, using cgroups to ensure that other processes in the system will not
> be scheduled on the benchmark CPUs. Transmit offloads and mergeable receive
> buffers are enabled, but guest_tso4 / guest_csum are explicitly disabled to
> force MTU-sized packets on the receiver.
>
> next-net trunk before 2613af0ed18a (PAGE_SIZE buf): 3861.08Gb/s
> net-next trunk (MTU 1500- packet uses two buf due to size bug): 4076.62Gb/s
> net-next trunk (MTU 1480- packet fits in one buf): 6301.34Gb/s
> net-next trunk w/ size fix (MTU 1500 - packet fits in one buf): 6445.44Gb/s
>
> Suggested-by: Eric Northup <digitaleric@google.com>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 01f4eb5..69fb225 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -36,7 +36,10 @@ module_param(csum, bool, 0444);
> module_param(gso, bool, 0444);
>
> /* FIXME: MTU in config. */
> -#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> +#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> +#define MERGE_BUFFER_LEN (ALIGN(GOOD_PACKET_LEN + \
> + sizeof(struct virtio_net_hdr_mrg_rxbuf), \
> + L1_CACHE_BYTES))
> #define GOOD_COPY_LEN 128
>
> #define VIRTNET_DRIVER_VERSION "1.0.0"
> @@ -314,10 +317,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> head_skb->dev->stats.rx_length_errors++;
> return -EINVAL;
> }
> - if (unlikely(len > MAX_PACKET_LEN)) {
> + if (unlikely(len > MERGE_BUFFER_LEN)) {
> pr_debug("%s: rx error: merge buffer too long\n",
> head_skb->dev->name);
> - len = MAX_PACKET_LEN;
> + len = MERGE_BUFFER_LEN;
> }
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> @@ -336,18 +339,17 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> if (curr_skb != head_skb) {
> head_skb->data_len += len;
> head_skb->len += len;
> - head_skb->truesize += MAX_PACKET_LEN;
> + head_skb->truesize += MERGE_BUFFER_LEN;
> }
> page = virt_to_head_page(buf);
> offset = buf - (char *)page_address(page);
> if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> put_page(page);
> skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> - len, MAX_PACKET_LEN);
> + len, MERGE_BUFFER_LEN);
> } else {
> skb_add_rx_frag(curr_skb, num_skb_frags, page,
> - offset, len,
> - MAX_PACKET_LEN);
> + offset, len, MERGE_BUFFER_LEN);
> }
> --rq->num;
> }
> @@ -383,7 +385,7 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> struct page *page = virt_to_head_page(buf);
> skb = page_to_skb(rq, page,
> (char *)buf - (char *)page_address(page),
> - len, MAX_PACKET_LEN);
> + len, MERGE_BUFFER_LEN);
> if (unlikely(!skb)) {
> dev->stats.rx_dropped++;
> put_page(page);
> @@ -471,11 +473,11 @@ static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
> struct skb_vnet_hdr *hdr;
> int err;
>
> - skb = __netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN, gfp);
> + skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp);
> if (unlikely(!skb))
> return -ENOMEM;
>
> - skb_put(skb, MAX_PACKET_LEN);
> + skb_put(skb, GOOD_PACKET_LEN);
>
> hdr = skb_vnet_hdr(skb);
> sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
> @@ -542,20 +544,20 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> int err;
>
> if (gfp & __GFP_WAIT) {
> - if (skb_page_frag_refill(MAX_PACKET_LEN, &vi->alloc_frag,
> + if (skb_page_frag_refill(MERGE_BUFFER_LEN, &vi->alloc_frag,
> gfp)) {
> buf = (char *)page_address(vi->alloc_frag.page) +
> vi->alloc_frag.offset;
> get_page(vi->alloc_frag.page);
> - vi->alloc_frag.offset += MAX_PACKET_LEN;
> + vi->alloc_frag.offset += MERGE_BUFFER_LEN;
> }
> } else {
> - buf = netdev_alloc_frag(MAX_PACKET_LEN);
> + buf = netdev_alloc_frag(MERGE_BUFFER_LEN);
> }
> if (!buf)
> return -ENOMEM;
>
> - sg_init_one(rq->sg, buf, MAX_PACKET_LEN);
> + sg_init_one(rq->sg, buf, MERGE_BUFFER_LEN);
> err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
> if (err < 0)
> put_page(virt_to_head_page(buf));
> --
> 1.8.4.1
^ permalink raw reply
* Re: [PATCH] rds: Error on offset mismatch if not loopback
From: Josh Hunt @ 2013-11-13 17:40 UTC (permalink / raw)
To: Venkat Venkatsubra, honli; +Cc: David Miller, jjolly, LKML, netdev
In-Reply-To: <23964ca1-e7cb-41c3-9da2-5bc1b2b0c014@default>
On Wed, Nov 13, 2013 at 9:16 AM, Venkat Venkatsubra
<venkat.x.venkatsubra@oracle.com> wrote:
>
>
> -----Original Message-----
> From: Josh Hunt [mailto:joshhunt00@gmail.com]
> Sent: Tuesday, November 12, 2013 10:25 PM
> To: David Miller
> Cc: jjolly@suse.com; LKML; Venkat Venkatsubra; netdev@vger.kernel.org
> Subject: Re: [PATCH] rds: Error on offset mismatch if not loopback
>
> On Tue, Nov 12, 2013 at 10:22 PM, Josh Hunt <joshhunt00@gmail.com> wrote:
>> On Sat, Sep 22, 2012 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
>>>
>>> From: John Jolly <jjolly@suse.com>
>>> Date: Fri, 21 Sep 2012 15:32:40 -0600
>>>
>>> > Attempting an rds connection from the IP address of an IPoIB
>>> > interface to itself causes a kernel panic due to a BUG_ON() being triggered.
>>> > Making the test less strict allows rds-ping to work without
>>> > crashing the machine.
>>> >
>>> > A local unprivileged user could use this flaw to crash the system.
>>> >
>>> > Signed-off-by: John Jolly <jjolly@suse.com>
>>>
>>> Besides the questions being asked of you by Venkat Venkatsubra, this
>>> patch has another issue.
>>>
>>> It has been completely corrupted by your email client, it has turned
>>> all TAB characters into spaces, making the patch useless.
>>>
>>> Please learn how to send a patch unmolested in the body of your
>>> email. Test it by emailing the patch to yourself, and verifying that
>>> you can in fact apply the patch you receive in that email.
>>> Then, and only then, should you consider making a new submission of
>>> this patch.
>>>
>>> Use Documentation/email-clients.txt for guidance.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-kernel" in the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at http://www.tux.org/lkml/
>>
>>
>> I think this issue was lost in the shuffle. It appears that redhat,
>> ubuntu, and oracle are maintaining local patches to resolve this:
>>
>> https://oss.oracle.com/git/?p=redpatch.git;a=commit;h=c7b6a0a1d8d63685
>> 2be130fa15fa8be10d4704e8
>> https://bugzilla.redhat.com/show_bug.cgi?id=822754
>> http://ubuntu.5.x6.nabble.com/CVE-2012-2372-RDS-local-ping-DOS-td49853
>> 88.html
>>
>> Given that Oracle has applied it I'll make the assumption that
>> Venkat's question was answered at some point.
>>
>> David - I can resubmit the patch with the proper signed-off-by and
>> formatting if you are willing to apply it unless John wants to try
>> again. I think it's time this got upstream.
>>
>> --
>> Josh
>
> Ugh.. hopefully resending with all the html crap removed...
>
> --
> Josh
>
> Hi Josh,
>
> No, I still didn't get an answer for how "off" could be non-zero in case of rds-ping to hit BUG_ON(off % RDS_FRAG_SIZE).
> Because, rds-ping uses zero byte messages to ping.
> If you have a test case that reproduces the kernel panic I can try it out and see how that can happen.
> The Oracle's internal code I checked doesn't have that patch applied.
>
> Venkat
No I don't have a test case. I came across this CVE while doing an
audit and noticed it was patched in Ubuntu's kernel and other distros,
but was not in the upstream kernel yet. Quick googling of lkml showed
that there were at least two attempts to get this patch upstream, but
both had issues due to not following the proper submission process:
https://lkml.org/lkml/2012/10/22/433
https://lkml.org/lkml/2012/9/21/505
>From my searching it appears the initial bug was found by someone at redhat:
https://bugzilla.redhat.com/show_bug.cgi?id=822754
I've added Li Honggang the reporter of this issue from Redhat to the
mail. Hopefully he can share his testcase.
and possibly requires certain hardware as Jay writes in the first link above:
"...some Infiniband HCAs(QLogic, possibly others) the machine will panic..."
I was referring to this oracle commit:
https://oss.oracle.com/git/?p=redpatch.git;a=commit;h=c7b6a0a1d8d636852be130fa15fa8be10d4704e8
I have no experience with this code. There were a few comments around
the reset and xmit fns about making sure the caller did certain things
if not they were racy, but I have no idea if that's coming into play
here.
--
Josh
^ permalink raw reply
* Re: oom-kill && frozen()
From: Peter Zijlstra @ 2013-11-13 17:42 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tejun Heo, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113170724.GA17739@redhat.com>
On Wed, Nov 13, 2013 at 06:07:24PM +0100, Oleg Nesterov wrote:
> 4. Finally, change try_to_wake_up() path to do
>
> - p->state = TASK_WAKING;
> + p->state &= ~state;
> + if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
> + return;
> + else
> + p->state = TASK_WAKING;
>
> IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
> then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
> try_to_wake_up(__TASK_FROZEN) to wake up.
> Tejun, Peter, do you think this makes any sense? I am just curious, but
> "selective wakeup" looks potentially useful.
I've never looked at any of this freeze stuff, so I cannot comment too
much. However we should be very careful not to add too much to relative
hot paths for the relative rare case of freezing stuff.
^ permalink raw reply
* Re: [PATCH net-next 4/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael S. Tsirkin @ 2013-11-13 17:42 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, Daniel Borkmann, virtualization,
Eric Dumazet, David S. Miller
In-Reply-To: <528325DC.3050801@redhat.com>
On Wed, Nov 13, 2013 at 03:10:20PM +0800, Jason Wang wrote:
> On 11/13/2013 06:21 AM, Michael Dalton wrote:
> > Commit 2613af0ed18a ("virtio_net: migrate mergeable rx buffers to page frag
> > allocators") changed the mergeable receive buffer size from PAGE_SIZE to
> > MTU-size, introducing a single-stream regression for benchmarks with large
> > average packet size. There is no single optimal buffer size for all workloads.
> > For workloads with packet size <= MTU bytes, MTU + virtio-net header-sized
> > buffers are preferred as larger buffers reduce the TCP window due to SKB
> > truesize. However, single-stream workloads with large average packet sizes
> > have higher throughput if larger (e.g., PAGE_SIZE) buffers are used.
> >
> > This commit auto-tunes the mergeable receiver buffer packet size by choosing
> > the packet buffer size based on an EWMA of the recent packet sizes for the
> > receive queue. Packet buffer sizes range from MTU_SIZE + virtio-net header
> > len to PAGE_SIZE. This improves throughput for large packet workloads, as
> > any workload with average packet size >= PAGE_SIZE will use PAGE_SIZE
> > buffers.
>
> Hi Michael:
>
> There's one concern with EWMA. How well does it handle multiple streams
> each with different packet size? E.g there may be two flows, one with
> 256 bytes each packet another is 64K. Looks like it can result we
> allocate PAGE_SIZE buffer for 256 (which is bad since the
> payload/truesize is low) bytes or 1500+ for 64K buffer (which is ok
> since we can do coalescing).
> >
> > These optimizations interact positively with recent commit
> > ba275241030c ("virtio-net: coalesce rx frags when possible during rx"),
> > which coalesces adjacent RX SKB fragments in virtio_net. The coalescing
> > optimizations benefit buffers of any size.
> >
> > Benchmarks taken from an average of 5 netperf 30-second TCP_STREAM runs
> > between two QEMU VMs on a single physical machine. Each VM has two VCPUs
> > with all offloads & vhost enabled. All VMs and vhost threads run in a
> > single 4 CPU cgroup cpuset, using cgroups to ensure that other processes
> > in the system will not be scheduled on the benchmark CPUs. Trunk includes
> > SKB rx frag coalescing.
> >
> > net-next trunk w/ virtio_net before 2613af0ed18a (PAGE_SIZE bufs): 14642.85Gb/s
> > net-next trunk (MTU-size bufs): 13170.01Gb/s
> > net-next trunk + auto-tune: 14555.94Gb/s
>
> Do you have perf numbers that just without this patch? We need to know
> how much EWMA help exactly.
Yes I'm curious too.
> >
> > Signed-off-by: Michael Dalton <mwdalton@google.com>
> > ---
> > drivers/net/virtio_net.c | 73 +++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 53 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 0c93054..b1086e0 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -27,6 +27,7 @@
> > #include <linux/if_vlan.h>
> > #include <linux/slab.h>
> > #include <linux/cpu.h>
> > +#include <linux/average.h>
> >
> > static int napi_weight = NAPI_POLL_WEIGHT;
> > module_param(napi_weight, int, 0444);
> > @@ -37,10 +38,8 @@ module_param(gso, bool, 0444);
> >
> > /* FIXME: MTU in config. */
> > #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> > -#define MERGE_BUFFER_LEN (ALIGN(GOOD_PACKET_LEN + \
> > - sizeof(struct virtio_net_hdr_mrg_rxbuf), \
> > - L1_CACHE_BYTES))
> > #define GOOD_COPY_LEN 128
> > +#define RECEIVE_AVG_WEIGHT 64
>
> Maybe we can make this as a module parameter.
I'm not sure it's useful - no one is likely to tune it in practice.
But how about a comment explaining how was the number chosen?
> >
> > #define VIRTNET_DRIVER_VERSION "1.0.0"
> >
> > @@ -79,6 +78,9 @@ struct receive_queue {
> > /* Chain pages by the private ptr. */
> > struct page *pages;
> >
> > + /* Average packet length for mergeable receive buffers. */
> > + struct ewma mrg_avg_pkt_len;
> > +
> > /* Page frag for GFP_ATOMIC packet buffer allocation. */
> > struct page_frag atomic_frag;
> >
> > @@ -302,14 +304,17 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
> > return skb;
> > }
> >
> > -static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > +static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb,
> > + struct page *head_page)
> > {
> > struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
> > struct sk_buff *curr_skb = head_skb;
> > + struct page *page = head_page;
> > char *buf;
> > - struct page *page;
> > - int num_buf, len, offset, truesize;
> > + int num_buf, len, offset;
> > + u32 est_buffer_len;
> >
> > + len = head_skb->len;
> > num_buf = hdr->mhdr.num_buffers;
> > while (--num_buf) {
> > int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> > @@ -320,7 +325,6 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > head_skb->dev->stats.rx_length_errors++;
> > return -EINVAL;
> > }
> > - truesize = max_t(int, len, MERGE_BUFFER_LEN);
> > if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> > struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> > if (unlikely(!nskb)) {
> > @@ -338,20 +342,38 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > if (curr_skb != head_skb) {
> > head_skb->data_len += len;
> > head_skb->len += len;
> > - head_skb->truesize += truesize;
> > + head_skb->truesize += len;
> > }
> > page = virt_to_head_page(buf);
> > offset = buf - (char *)page_address(page);
> > if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> > put_page(page);
> > skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> > - len, truesize);
> > + len, len);
> > } else {
> > skb_add_rx_frag(curr_skb, num_skb_frags, page,
> > - offset, len, truesize);
> > + offset, len, len);
> > }
> > --rq->num;
> > }
> > + /* All frags before the last frag are fully used -- for those frags,
> > + * truesize = len. Use the size of the most recent buffer allocation
> > + * from the last frag's page to estimate the truesize of the last frag.
> > + * EWMA with a weight of 64 makes the size adjustments quite small in
> > + * the frags allocated on one page (even a order-3 one), and truesize
> > + * doesn't need to be 100% accurate.
> > + */
> > + if (page) {
> > + est_buffer_len = page_private(page);
> > + if (est_buffer_len > len) {
> > + u32 truesize_delta = est_buffer_len - len;
> > +
> > + curr_skb->truesize += truesize_delta;
> > + if (curr_skb != head_skb)
> > + head_skb->truesize += truesize_delta;
> > + }
>
> Is there a chance that est_buffer_len was smaller than or equal with len?
> > + }
> > + ewma_add(&rq->mrg_avg_pkt_len, head_skb->len);
> > return 0;
> > }
> >
> > @@ -382,16 +404,21 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> > skb_trim(skb, len);
> > } else if (vi->mergeable_rx_bufs) {
> > struct page *page = virt_to_head_page(buf);
> > - int truesize = max_t(int, len, MERGE_BUFFER_LEN);
> > + /* Use an initial truesize of 'len' bytes for page_to_skb --
> > + * receive_mergeable will fixup the truesize of the last page
> > + * frag if the packet is non-linear (> GOOD_COPY_LEN bytes).
> > + */
> > skb = page_to_skb(rq, page,
> > (char *)buf - (char *)page_address(page),
> > - len, truesize);
> > + len, len);
> > if (unlikely(!skb)) {
> > dev->stats.rx_dropped++;
> > put_page(page);
> > return;
> > }
> > - if (receive_mergeable(rq, skb)) {
> > + if (!skb_is_nonlinear(skb))
> > + page = NULL;
> > + if (receive_mergeable(rq, skb, page)) {
> > dev_kfree_skb(skb);
> > return;
> > }
> > @@ -540,24 +567,29 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
> > static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> > {
> > struct virtnet_info *vi = rq->vq->vdev->priv;
> > + const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > struct page_frag *alloc_frag;
> > char *buf;
> > - int err, len, hole;
> > + int err, hole;
> > + u32 buflen;
> >
> > + buflen = hdr_len + clamp_t(u32, ewma_read(&rq->mrg_avg_pkt_len),
> > + GOOD_PACKET_LEN, PAGE_SIZE - hdr_len);
> > + buflen = ALIGN(buflen, L1_CACHE_BYTES);
> > alloc_frag = (gfp & __GFP_WAIT) ? &vi->sleep_frag : &rq->atomic_frag;
> > - if (unlikely(!skb_page_frag_refill(MERGE_BUFFER_LEN, alloc_frag, gfp)))
> > + if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, gfp)))
> > return -ENOMEM;
> > buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> > get_page(alloc_frag->page);
> > - len = MERGE_BUFFER_LEN;
> > - alloc_frag->offset += len;
> > + alloc_frag->offset += buflen;
> > + set_page_private(alloc_frag->page, buflen);
>
> Not sure this is accurate, since buflen may change and several frags may
> share a single page. So the est_buffer_len we get in receive_mergeable()
> may not be the correct value.
> > hole = alloc_frag->size - alloc_frag->offset;
> > - if (hole < MERGE_BUFFER_LEN) {
> > - len += hole;
> > + if (hole < buflen) {
> > + buflen += hole;
> > alloc_frag->offset += hole;
> > }
> >
> > - sg_init_one(rq->sg, buf, len);
> > + sg_init_one(rq->sg, buf, buflen);
> > err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
> > if (err < 0)
> > put_page(virt_to_head_page(buf));
> > @@ -1475,6 +1507,7 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
> > napi_weight);
> >
> > sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
> > + ewma_init(&vi->rq[i].mrg_avg_pkt_len, 1, RECEIVE_AVG_WEIGHT);
> > sg_init_table(vi->sq[i].sg, ARRAY_SIZE(vi->sq[i].sg));
> > }
> >
^ 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