* Re: [PATCH 0/8 net-next-2.6] udp: optimisations
From: David Miller @ 2009-11-09 4:55 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, lgrijincu, opurdila
In-Reply-To: <4AF72738.7020606@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 08 Nov 2009 21:16:56 +0100
> This patch series address UDP scalability problems, we failed to solve in 2007
> (commit 6aaf47fa48d3c44 INET : IPV4 UDP lookups converted to a 2 pass algo)
> we had to revert a bit later.
Looks great, all applied, thanks Eric.
I would even go so far as to say that the cutoff to the second hash
table should be even lower than 10, like maybe 4 or 5.
^ permalink raw reply
* Re: [RFC, PATCH 0/7] net, compat_ioctl: move handlers to net/socket.c
From: David Miller @ 2009-11-09 4:55 UTC (permalink / raw)
To: arnd; +Cc: linux-kernel, hch, netdev, dwmw2
In-Reply-To: <200911082231.48060.arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Sun, 8 Nov 2009 22:31:47 +0100
> Any opinion on how to proceed with the ATM stuff? From my
> point of view, I'm fine with having moved it out of fs/compat_ioctl.c,
> but it's still a bit silly to have two half-complete implementations.
I haven't looked at that stuff yes, you can be sure it's in
my queue :-)
^ permalink raw reply
* Re: [PATCH 1/2] net, compat_ioctl: fix SIOCGMII ioctls
From: David Miller @ 2009-11-09 4:56 UTC (permalink / raw)
To: arnd; +Cc: linux-kernel, hch, netdev
In-Reply-To: <200911082234.14031.arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Sun, 8 Nov 2009 22:34:13 +0100
> SIOCGMIIPHY and SIOCGMIIREG return data through ifreq,
> so it needs to be converted on the way out as well.
>
> SIOCGIFPFLAGS is unused, but has the same problem in theory.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2] net/compat_ioctl: support SIOCWANDEV
From: David Miller @ 2009-11-09 4:57 UTC (permalink / raw)
To: arnd; +Cc: khc, dwalker, linux-kernel, hch, netdev
In-Reply-To: <200911082239.24698.arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Sun, 8 Nov 2009 22:39:24 +0100
> This adds compat_ioctl support for SIOCWANDEV, which has
> always been missing.
>
> The definition of struct compat_ifreq was missing an
> ifru_settings fields that is needed to support SIOCWANDEV,
> so add that and clean up the whitespace damage in the
> struct definition.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] xfrm: SAD entries do not expire correctly after suspend-resume
From: David Miller @ 2009-11-09 4:58 UTC (permalink / raw)
To: ypolyans; +Cc: netdev, peterz, yoshfuji, tglx, mingo
In-Reply-To: <20091108211249.2ecdfd38@penta.localdomain>
From: Yury Polyanskiy <ypolyans@princeton.edu>
Date: Sun, 8 Nov 2009 21:12:49 -0500
>
> This fixes the following bug in the current implementation of
> net/xfrm: SAD entries timeouts do not count the time spent by the machine
> in the suspended state. This leads to the connectivity problems because
> after resuming local machine thinks that the SAD entry is still valid, while
> it has already been expired on the remote server.
...
> Signed-off-by: Yury Polyanskiy <polyanskiy@gmail.com>
Applied to net-next-2.6, thanks for following up on this.
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the net-current tree
From: David Miller @ 2009-11-09 4:59 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel, wg, haas
In-Reply-To: <20091109130323.5dcc7829.sfr@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Mon, 9 Nov 2009 13:03:23 +1100
> Hi all,
>
> Today's linux-next merge of the net tree got a conflict in
> drivers/net/can/usb/ems_usb.c between commit
> 2b2072e902848a63168570f500a5726744b3873a ("ems_usb: Fix byte order issues
> on big endian machines") from the net-current tree and commit
> 7b6856a0296a8f187bb88ba31fa83a08abba7966 ("can: provide library functions
> for skb allocation") from the net tree.
>
> Just context changes. I fixed it up (see below) and can carry the fix
> for a while.
I'll do a merge and resolve this, thanks Stephen!
^ permalink raw reply
* Re: [PATCH 2/2 net-next-2.6] au1000-eth: convert to platform_driver model
From: David Miller @ 2009-11-09 5:02 UTC (permalink / raw)
To: florian; +Cc: linux-mips, ralf, netdev
In-Reply-To: <200911081542.12219.florian@openwrt.org>
From: Florian Fainelli <florian@openwrt.org>
Date: Sun, 8 Nov 2009 15:42:11 +0100
> This patch converts the au1000-eth driver to become a full
> platform-driver as it ought to be. We now pass PHY-speficic
> configurations through platform_data but for compatibility
> the driver still assumes the default settings (search for PHY1 on
> MAC0) when no platform_data is passed. Tested on my MTX-1 board.
>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
Ralf, feel free to merge this yourself since it depends upon
the previous Alchemy platform patch:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* away but active for next 6 days.
From: David Miller @ 2009-11-09 5:04 UTC (permalink / raw)
To: netdev; +Cc: linux-ide, sparclinux
I will be away in New York City from Monday night until Saturday.
I intend to be as responsive as I was this past summer when I spent
nearly 2 months there :-)
I have one IDE fix, 2 or 3 Sparc fixes, and a moderately sized handful
of networking fixes sitting in my trees which I'll send pull requests
out for tonight.
^ permalink raw reply
* [GIT]: Networking
From: David Miller @ 2009-11-09 5:32 UTC (permalink / raw)
To: torvalds; +Cc: akpm, netdev, linux-kernel
1) Wireless fixes, including a revert for something that added as
many bugs as it fixed. Via John Linville.
2) Rose and DecNET leaks device references. CAN protocol doesn't
lock device list accesses properly. All from Eric Dumazet.
3) TX of ixgbe can hang with ioatdma loded, fix from Don Skidmore.
4) be2net fixes for flow control programming on resume, the timing
of sending configuration commands to the chip, and a broken
status shift macro define, from Ajit Khaparde and Sathya Perla.
5) A queue of ISDN fixes via Andrew Morton.
6) Two netfilter bug fixes (Patrick McHardy has been offline for a month
or so these are the only two netfilter fixes that have gone in in
that time), from Jan Engelhardt and Jozsef Kadlecsik.
7) When PMTU discovery is off, we handle DF packets improperly in
IPIP tunnel driver. Fix from Herbert Xu.
8) fsl_pq_mdio is missing MODULE_LICENSE() tag, from Sebastian Siewior.
9) virtio_net section mismatch fix from Uwe Kleine-König.
10) IXGBE uses wrong gso_max_size on 82599 chips when DCB is enabled.
It also checks TXOFF status incorrectly when DCB is enabled. Both
fixed from Yi Zou.
11) CAN protocol needs to provide ->get_size() operation otherwise
netlink SKBs aren't sized for it correctly. From Wolfgang
Grandegger.
Please pull, thanks a lot.
The following changes since commit 7c9abfb884b8737f0afdc8a88bcea77526f0da87:
Linus Torvalds (1):
Merge branch 'kvm-updates/2.6.32' of git://git.kernel.org/pub/scm/virt/kvm/kvm
are available in the git repository at:
master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master
Ajit Khaparde (2):
be2net: fix to set proper flow control on resume
be2net: Bug fix to send config commands to hardware after netdev_register
Dan Carpenter (1):
misdn: Fix reversed 'if' in st_own_ctrl
David S. Miller (1):
Merge branch 'master' of git://git.kernel.org/.../linville/wireless-2.6
Don Skidmore (1):
ixgbe: fix traffic hangs on Tx with ioatdma loaded
Eric Dumazet (3):
rose: device refcount leak
decnet: netdevice refcount leak
can: should not use __dev_get_by_index() without locks
Finn Thain (1):
macsonic: fix crash on PowerBook 520
Herbert Xu (1):
ipip: Fix handling of DF packets when pmtudisc is OFF
Jan Engelhardt (1):
netfilter: xt_connlimit: fix regression caused by zero family value
Jiri Slaby (4):
isdn: hisax: Fix lock imbalance.
isdn: eicon: Use offsetof
isdn: eicon: Return on error
NET: cassini, fix lock imbalance
Joe Perches (1):
MAINTAINERS: Add git net-next-2.6
John W. Linville (1):
Revert "ipw2200: fix oops on missing firmware"
Jozsef Kadlecsik (1):
netfilter: nf_nat: fix NAT issue in 2.6.30.4+
Larry Finger (1):
rtl8187: Fix kernel oops when device is removed when LEDS enabled
Martin Michlmayr (1):
hisax: remove bad udelay call to fix build error on ARM
Roel Kluin (2):
isdn: hisax: Fix test in waitforxfw
isdn: hfc_usb: Fix read buffer overflow
Ron Mercer (2):
qlge: Fix early exit from mbox cmd complete wait.
qlge: Set PCIe reset type for EEH to fundamental.
Sathya Perla (1):
be2net: Fix CQE_STATUS_EXTD_SHIFT define
Sean Cross (1):
rt2x00: Don't queue ieee80211 work after USB removal
Sebastian Haas (1):
ems_usb: Fix byte order issues on big endian machines
Sebastian Siewior (1):
net/fsl_pq_mdio: add module license GPL
Stephen Hemminger (1):
bridge: prevent bridging wrong device
Tonyliu (1):
DaVinci EMAC: correct param for ISR
Torgny Johansson (1):
cdc_ether: additional Ericsson MBM PID's to the whitelist
Uwe Kleine-König (1):
virtio_net: rename driver struct to please modpost
Wolfgang Grandegger (1):
can: fix WARN_ON dump in net/core/rtnetlink.c:rtmsg_ifinfo()
Yi Zou (2):
ixgbe: Fix gso_max_size for 82599 when DCB is enabled
ixgbe: Fix checking TFCS register for TXOFF status when DCB is enabled
MAINTAINERS | 1 +
drivers/isdn/hardware/eicon/maintidi.c | 5 +-
drivers/isdn/hardware/eicon/message.c | 18 ++--
drivers/isdn/hisax/amd7930_fn.c | 1 +
drivers/isdn/hisax/diva.c | 2 +-
drivers/isdn/hisax/elsa_ser.c | 22 +++---
drivers/isdn/hisax/hfc_usb.c | 4 +-
drivers/isdn/hisax/hscx_irq.c | 2 +-
drivers/isdn/hisax/icc.c | 1 +
drivers/isdn/mISDN/stack.c | 2 +-
drivers/net/benet/be.h | 2 +
drivers/net/benet/be_cmds.h | 2 +-
drivers/net/benet/be_ethtool.c | 8 +-
drivers/net/benet/be_main.c | 28 ++++---
drivers/net/can/dev.c | 17 ++++
drivers/net/can/usb/ems_usb.c | 5 +-
drivers/net/cassini.c | 5 +-
drivers/net/davinci_emac.c | 2 +-
drivers/net/fsl_pq_mdio.c | 1 +
drivers/net/ixgbe/ixgbe_main.c | 84 +++++++++++++++++--
drivers/net/macsonic.c | 117 +++++++++++++-------------
drivers/net/qlge/qlge_main.c | 2 +
drivers/net/qlge/qlge_mpi.c | 2 +-
drivers/net/usb/Kconfig | 2 +-
drivers/net/usb/cdc_ether.c | 42 +++++++++-
drivers/net/virtio_net.c | 6 +-
drivers/net/wireless/ipw2x00/ipw2100.c | 5 +-
drivers/net/wireless/ipw2x00/ipw2200.c | 2 -
drivers/net/wireless/ipw2x00/libipw.h | 1 -
drivers/net/wireless/ipw2x00/libipw_module.c | 14 +--
drivers/net/wireless/rt2x00/rt2x00dev.c | 4 +-
drivers/net/wireless/rt2x00/rt2x00link.c | 11 ++-
drivers/net/wireless/rt2x00/rt2x00usb.c | 9 ++-
drivers/net/wireless/rtl818x/rtl8187_leds.c | 4 +-
include/net/netfilter/nf_conntrack.h | 8 +-
include/net/netfilter/nf_nat_helper.h | 4 +
net/bridge/br_if.c | 6 +-
net/can/bcm.c | 19 +++--
net/decnet/sysctl_net_decnet.c | 7 +-
net/ipv4/ipip.c | 32 ++++----
net/ipv4/netfilter/nf_nat_core.c | 3 +
net/ipv4/netfilter/nf_nat_helper.c | 34 +++++---
net/netfilter/nf_conntrack_core.c | 8 ++
net/netfilter/nf_conntrack_proto_tcp.c | 64 ++++++--------
net/netfilter/xt_connlimit.c | 10 +--
net/rose/rose_route.c | 16 ++--
46 files changed, 407 insertions(+), 237 deletions(-)
^ permalink raw reply
* Re: [PATCH 0/8 net-next-2.6] udp: optimisations
From: Eric Dumazet @ 2009-11-09 5:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lgrijincu, opurdila
In-Reply-To: <20091108.205522.185944441.davem@davemloft.net>
David Miller a écrit :
>
> Looks great, all applied, thanks Eric.
Thanks David, I'll make the remaining patches too.
>
> I would even go so far as to say that the cutoff to the second hash
> table should be even lower than 10, like maybe 4 or 5.
Probably, but we want to avoid the secondary way if possible,
as this path might have to traverse two different chains.
(total of three cache line accesses to only take a look at chains head/count)
Maybe we can change the heuristic to take into account this like that :
if (hslot->count > 4) {
...
if (hslot->count < hslot2->count * 2)
goto begin_primary_hash_lookup;
This is tuning, and needs benchmarking.
^ permalink raw reply
* Re: RFC: ethtool support for n-tuple filter programming
From: Peter P Waskiewicz Jr @ 2009-11-09 5:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20091107.202712.222562359.davem@davemloft.net>
On Sat, 2009-11-07 at 21:27 -0700, David Miller wrote:
> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Date: Fri, 06 Nov 2009 10:57:21 -0800
>
> > Any comments, thoughts, suggestions, or ideas are welcome.
>
> We can use the existing datastructures and defines used for
> ETHTOOL_GRX* for new ethtool commands that do filtering.
>
> NIU can filter on these tuples too.
Are you suggesting to extend the flow hash stuff that is already in
ethtool (same ioctl)? If so, that makes sense. I'll go ahead and get
some patches together for review.
Thanks,
-PJ
^ permalink raw reply
* Re: Doubt in implementations of mean loss interval at sender side
From: Gerrit Renker @ 2009-11-09 6:09 UTC (permalink / raw)
To: Ivo Calado; +Cc: dccp, netdev
In-Reply-To: <cb00fa210911051603w6fb8de32qd7ebf37ce78408f7@mail.gmail.com>
| > To sum up, here is whay I think is minimally required to satisfy the union
| > of RFC 4340, 4342, 4828, 5348, and 5622:
| >
| > struct tfrc_tx_packet_info {
| > u64 seqno:48,
| > is_ect0:1,
| > is_data_packet:1,
| > is_in_loss_interval:1;
| > u32 send_time;
| > u32 rtt_estimate;
| > struct tfrc_tx_packet_info *next; /* FIFO */
| > };
| >
| > That would be a per-packet storage cost of about 16 bytes, plus the pointer
| > (8 bytes on 64-bit architectures). One could avoid the pointer by defining a
| > u64 base_seqno;
| > and then
| > struct tfrc_tx_packet_info[some constant here];
| > and then index the array relative to the base_seqno.
| >
|
| Yes, I believe that struct is enough too. But how long would be necessary
| the struct array to be?
|
The problem is the same as with Ack Vectors - the array (or list) can grow
arbitrarily large. You made a good reply, since all the questions are
inter-related. The first two I see here are
1) the choice of data structure (array or list)
2) the design of a garbage-collector
This includes your point from above, about the maximum size. To draw the
analogy to Ack Vectors, at the moment they use a fixed size. On certain
mediums (WiFi) there exist situations where even that fixed limit is
reached, causing an overflow with Ack Vectors that have reached a size
of 2 * 253 = 506 bytes.
Looking after old data of sent packets is similar, the main difference I
see that at some stage "unused" old entries need to be collected, to avoid
the overflow problem which occurs when using a fixed-size structure.
I find that 'Acknowledgments of Acknowledgments' is a bad idea, since it
means implementing reliable delivery over unreliable transport; on the
one hand DCCP is designed to be unreliable, but here suddenly is a break
with that design decision.
So point (2) will probably mean coming up with some guessed heuristics
that will work for most scenarios, but may fail in others.
This is why I am not a big fan of the sender-based solution: to solve (2) and
your question from above requires a lot of testing of the garbage-collection
and book-keeping algorithms, rather than on the actual congestion control.
One can spend a lot of time going over these issues, but of what use is the
most ingenious data structure if the overall protocol behaviour does not
deliver a useful performance to users of the protocol?
| > IIb) Further remarks
| > --------------------
| > At first sight it would seem that storing the RTT also solves the problem
| > of inaccurate RTTs used at the receiver. Unfortunately, this is not the
| > case. X_recv is sampled over intervals of varying length which may or may
| > not equal the RTT. To factor out the effect of window counters, the sender
| > would need to store the packet size as well and would need to use rather
| > complicated computations - an ugly workaround.
|
| I didn't understand how the packet size would help and what
| computations are needed.
|
The above still refers to the earlier posting about letting the sender
supply the RTT estimate R_i in packet `i' as defined in RFC 5348, 3.2.1.
Though the same section later suggests that a coarse-grained timestamp
is sufficient, in practice the inaccuracy of the RTT means inaccurate
X_recv, and as a consequence sub-optimal protocol performance.
The problem is that the algorithm from RFC 4342, 8.1 assumes that the
rate of change of the window counter also relates to the change of
packet spacing (the difference between the T_i packe arrivel times).
Howver, especially when using high-speed (1/10 Gbit) networking, this
assumption often does not hold in practice. Packets are sent by the network
card in bunches, or intervening switches/routers cause a compression of
packet inter-arrival times. Hence it is perfectly possible that a bundle of
packets with different Window Counter CCVal values arrive at virtually
the same time. For instance, on a 100Mbs ethernet I have seen spikes of
X_recv of up to 2-3 Gbits/sec. Several orders of magnitude from the
real packet speed (not to mention the unrealistic value).
So the question above was asking whether there is a way for the sender
to "compute away" the inaccuracies reported by the receiver. Your reply
confirms my doubts that doing this is probably not possible.
To clarify, I was asking whether it would be possible for the sender to
perform step (2) of RFC 5348, 6.2; to compensate for the fact that the
receiver does not have a reliable RTT estimate.
For example, when receiving feedback for packet `i', it would iterate
through the list/array, going back over as many packets as are covered
by the send time T_i of packet `i' minus the RTT estimate R_i at that
time, sum their packet sizes, and from that value recompute X_recv.
This is a bit complicated if the garbage-collector has already purged
older entries, so part of the garbage collector would probably have
to watch over acknowledged packets. I add this as item
3) validate X_recv
to the above running list of book-keeping items done at the sender.
| > One thing I stumbled across while reading your code was the fact that RFC 4342
| > leaves it open as to how many Loss Intervals to send: on the one hand it follows
| > the suggestion of RFC 5348 to use 1+NINTERVAL=9, but on the other hand it does
| > not restrict the number of loss intervals. Also RFC 5622 does not limit the
| > number of Loss Intervals / Data Dropped options.
| >
| > If receiving n > 9 Loss Intervals, what does the sender do with the n-9 older
| > intervals? There must be some mechanism to stop these options from growing
| > beyond bounds, so it needs to store also which loss intervals have been
| > acknowledged, introducing the "Acknowledgment of Acknowledgments"
| > problem.
|
| In RFC 4342 section 8.6 it says that the limit of loss interval data
| to send is 28, and RFC 5622 8.7 says 84 for dropped packets option.
| But I don't see why to send so many data in these options.
| Yes, the most recent 9 loss intervals are required to be reported,
| except if the sender acknowledged previous sent loss intervals, so in
| that case only one is required, the open interval.
| And we can avoid the "Acknowledgment of Acknowledgments" if we always send
| the required 9 loss intervals, I think.
|
| > A second point is how to compute the loss event rate when n > 9. It seems
| > that this would mean grinding through all loss intervals using a window
| > of 9. If that is the case, the per-packet-computation costs become very
| > expensive.
|
| RFC 4342 section 8.6 suggests that only 9 loss intervals are required
| anyway. And I believe that's enough for the computation of current
| mean loss interval. What do you think?
|
Yes, absolutely, I am completely in favour of this very sensible suggestion.
If people really must experiment with such outdated data, that could be
done in truly experimental patches. Especially since RFC 5348 normatively
recommends a value of n = 8 in section 5.4. And we are saving further
headaches about the book-keeping/garbage collection of old data.
| > II) Computational part of the implementation
| > --------------------------------------------
| > If only Loss Intervals alone are used, only these need to be verified
| > before being used to alter the sender behaviour.
| >
| > But when one or more other DCCP options also appear, the verification is
| > * intra: make sure each received option is in itself consistent,
| > * inter: make sure options are mutually consistent.
| >
| > The second has a combinatorial effect, i.e. n! verifications for n options.
| >
<snip>
|
| Yes, there's a combinatorial problem in checking the options for consistence.
| But, what if we find out that some option doesn't match against others?
| What action would be taken?
I add this as
4) define policy for dealing with options that are not mutually consistent
| First, what can cause the receiver to send inconsistent options?
| A bad implementation only?
Yes I think that a bad implementation (whether on purpose or not) would be
the main cause, since header options are protected even if partial
checksums are used (RFC 4340, 9.2).
But there is also the benign case mentioned at the end of RFC 4342, 9.2,
where a receiver collapses multiple losses into a single loss event, i.e.
5) validate received Loss Intervals and regroup the receiver-based
information if necessary, without interpreting this as attempted
receiver misbehaviour.
| Accordingly to ecn nonce echo sum algorithm, if a receiver is found to be
| lying about loss or to be bad implemented, the sender adjusts the send rate
| as if loss were perceived.
| Can we do the same in this situation? If so, can we skip checking options
| between them and only check ecn nonce sum?
This is difficult since Ack Vectors and Loss Intervals use different
definitions of ECN Nonce sum (last paragraph in RFC 4342, 9.1), i.e. we have
6) separate algorithms to compute Ack Vector/Loss Intervals ECN Nonce sum.
With regard to (5) above, your suggestion gives
7) validate options, on mismatch other than (5) only validate ECN nonce.
| If some option is wrong it show more loss (or any worse situation for the
| receiver) or conceals loss. In the first case, I don't believe we need to care,
| and in the second, the ecn nonce sum can reveal the bad acting of the receiver.
Yes you are right, we need not worry if a receiver reports a higher loss rate
than the verification done by the sender (which recomputes the data that the
receiver already has computed) calculates.
But for the second case, there is no guarantee to catch a misbehaving
receiver, only a 50% chance at the end of many computations.
RFC 4342, 9 suggests one way of verifying Loss Intervals / Ack Vectors:
5) occasionally do not send a packet, or send packet out of order.
This increases complexity of book-keeping, the sender needs to keep track
which of the sent packets was a fake send/drop. It also requires an algorithm
to iterate over the sender data structures in order to find out whether the
reasoning of the receiver is sane. I have doubts whether this can be done
without sacrificing the performance of the in-kernel sender side.
| > III) Closing remarks in favour of receiver-based implementation
| > ---------------------------------------------------------------
| > Finally, both RFC 4342 and RFC 5622 do not explicitly discard the
| > possibility of using a receiver-based implementation. Quoting
| > RFC 4342, 3.2: "If it prefers, the sender can also use a loss event
| > rate calculated and reported by the receiver."
| > Furthermore, the revised TFRC specification points out in section 7
| > the advantages that a receiver-based implementation has:
| > * it does not mandate reliable delivery of packet loss data;
| > * it is robust against the loss of feedback packets;
| > * better suited for scalable server design.
| >
| > Quite likely, if the server does not have to store and validate a mass
| > of data, it is also less prone to be toppled by DoS attacks.
|
| You're right. But what the RFC's says about it is almost exactly the
| opposite, isn't? What can we do about it? I like the receiver-based design,
| but I believe that loss intervals are interesting, mostly because of
| receiver behavior verification.
|
While writing the above reply, I was amazed to see how much of the computation
that has already been done at the receiver needs to be done again at the sender,
ust in order to be able to verify the data.
To me this seems very inefficient.
Moreover, the biggest danger I see here is spending a lot of time with the
details of sender book-keeping and verification, just to then see that the
performance of CCID-3/4 in practice turns out to be below the standards
acceptable to even modest users.
I think it is clearly better to prefer the simplest possible implementation
in such cases, to better debug live protocol performance.
In particular, since CCID-4 is designed to be an experimental protocol
(i.e. if it works, RFC 5622 may mature into a Proposed Standard, if not,
it might be superseded by a different specification).
And I think that testing the actual user performance has the highest priority.
The literature on the subject is almost exclusively done on experiences in
ns-2 userland. Almost no Internet experiments at all have been done with DCCP.
This is because the IPPROTO = 33 identifier needs to be entered
especially into a firewall, which opens holes that firewall
administrators don't like to open (unless the firewall is based on
a recent Linux kernel, opening all ports for IP protocol identifier
33 is the only way of allowing DCCP traffic in/out of a firewall).
In addition, most people use NAT at home, putting another obstacle
on experiments. The result is then that tests are done in a lab testbed
or in virtualisation - emulated networks.
To conclude, I still think that the simpler, receiver-based implementation
gives a better start. A 'perfect' receiver implementation is also a good
reference point to start protocol evaluation: if the performance is bad
despite getting things right at the receiver, then other parts of the
protocol need investigation/improvement.
^ permalink raw reply
* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Rusty Russell @ 2009-11-09 6:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <20091108113516.GA19016@redhat.com>
On Sun, 8 Nov 2009 10:05:16 pm Michael S. Tsirkin wrote:
> On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> > > +/* Caller must have TX VQ lock */
> > > +static void tx_poll_stop(struct vhost_net *net)
> > > +{
> > > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > > + return;
> >
> > likely? Really?
>
> Hmm ... yes. tx poll stop is called on each packet (as long as we do not
> fill up 1/2 backend queue), the first call will stop polling
> the rest checks state and does nothing.
>
> This is because we normally do not care when the message has left the
> queue in backend device: we tell backend to send it and forget. We only
> start polling when backend tx queue fills up.
OK, good.
> > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> > > +{
> > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > > + sizeof(struct virtio_net_hdr) : 0;
> > > + int i;
> > > + mutex_lock(&n->dev.mutex);
> > > + n->dev.acked_features = features;
> >
> > Why is this called "acked_features"? Not just "features"? I expected
> > to see code which exposed these back to userspace, and didn't.
>
> Not sure how do you mean. Userspace sets them, why
> does it want to get them exposed back?
There's something about the 'acked' which rubs me the wrong way.
"enabled_features" is perhaps a better term than "acked_features"; "acked"
seems more a user point-of-view, "enabled" seems more driver POV?
set_features matches your ioctl names, but it sounds like a fn name :(
It's marginal. And 'features' is shorter than both.
> > > + switch (ioctl) {
> > > + case VHOST_SET_VRING_NUM:
> >
> > I haven't looked at your userspace implementation, but does a generic
> > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> > sense? It'd be simpler here,
>
> Not by much though, right?
>
> > but not sure if it'd be simpler to use?
>
> The problem is with VHOST_SET_VRING_BASE as well. I want it to be
> separate because I want to make it possible to relocate e.g. used ring
> to another address while ring is running. This would be a good debugging
> tool (you look at kernel's used ring, check descriptor, then update
> guest's used ring) and also possibly an extra way to do migration. And
> it's nicer to have vring size separate as well, because it is
> initialized by host and never changed, right?
Actually, this looks wrong to me:
+ case VHOST_SET_VRING_BASE:
...
+ vq->avail_idx = vq->last_avail_idx = s.num;
The last_avail_idx is part of the state of the driver. It needs to be saved
and restored over susp/resume. The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but
missed debugging opportunities as well as man-in-the-middle issues like this
one). I had a patch which put this field at the end of the ring, I might
resurrect it to avoid this problem. This is backwards compatible with all
implementations. See patch at end.
I would drop avail_idx altogether: get_user is basically free, and simplifies
a lot. As most state is in the ring, all you need is an ioctl to save/restore
the last_avail_idx.
> We could merge DESC, AVAIL, USED, and it will reduce the amount of code
> in userspace. With both base, size and fds separate, it seemed a bit
> more symmetrical to have desc/avail/used separate as well.
> What's your opinion?
Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.
> > For future reference, this is *exactly* the kind of thing which would have
> > been nice as a followup patch. Easy to separate, easy to review, not critical
> > to the core.
>
> Yes. It's not too late to split it out though: should I do it yet?
Only if you're feeling enthused. It's lightly reviewed now.
Cheers,
Rusty.
virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where
you're up to in consuming the ring. However, to completely understand
what's going on from the outside, this information must be exposed.
For example, if you want to save and restore a virtio_ring, but you're
not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number
of pages and there's hundreds of bytes of padding after the avail ring
and the used ring, whatever the number of descriptors (which must be a
power of 2).
We add a feature bit so the guest can tell the host that it's writing
out the current value there, if it wants to use that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
drivers/virtio/virtio_ring.c | 23 +++++++++++++++--------
include/linux/virtio_ring.h | 12 +++++++++++-
2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -71,9 +71,6 @@ struct vring_virtqueue
/* Number we've added since last sync. */
unsigned int num_added;
- /* Last used index we've seen. */
- u16 last_used_idx;
-
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
static inline bool more_used(const struct vring_virtqueue *vq)
{
- return vq->last_used_idx != vq->vring.used->idx;
+ return vring_last_used(&vq->vring) != vq->vring.used->idx;
}
static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ struct vring_used_elem *u;
void *ret;
unsigned int i;
@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
return NULL;
}
- i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
- *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+ u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+ i = u->id;
+ *len = u->len;
+ /* Make sure we don't reload i after doing checks. */
+ rmb();
if (unlikely(i >= vq->vring.num)) {
BAD_RING(vq, "id %u out of range\n", i);
@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
/* detach_buf clears data, so grab it now. */
ret = vq->data[i];
detach_buf(vq, i);
- vq->last_used_idx++;
+ vring_last_used(&vq->vring)++;
+
END_USE(vq);
return ret;
}
@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
vq->vq.name = name;
vq->notify = notify;
vq->broken = false;
- vq->last_used_idx = 0;
vq->num_added = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
+ /* We publish indices whether they offer it or not: if not, it's junk
+ * space anyway. But calling this acknowledges the feature. */
+ virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
/* No callback? Tell other side not to bother us. */
if (!callback)
vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_PUBLISH_INDICES:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,9 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc
{
@@ -87,6 +90,7 @@ struct vring {
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
+ * __u16 last_used_idx;
*
* // Padding to the next align boundary.
* char pad[];
@@ -95,6 +99,7 @@ struct vring {
* __u16 used_flags;
* __u16 used_idx;
* struct vring_used_elem used[num];
+ * __u16 last_avail_idx;
* };
*/
static inline void vring_init(struct vring *vr, unsigned int num, void *p,
@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
{
return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
+ align - 1) & ~(align - 1))
- + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+ + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
}
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa. These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
#ifdef __KERNEL__
#include <linux/irqreturn.h>
struct virtio_device;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC: ethtool support for n-tuple filter programming
From: David Miller @ 2009-11-09 6:38 UTC (permalink / raw)
To: peter.p.waskiewicz.jr; +Cc: netdev
In-Reply-To: <1257745762.2609.5.camel@ppwaskie-mobl2>
From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Sun, 08 Nov 2009 21:49:21 -0800
> On Sat, 2009-11-07 at 21:27 -0700, David Miller wrote:
>> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Date: Fri, 06 Nov 2009 10:57:21 -0800
>>
>> > Any comments, thoughts, suggestions, or ideas are welcome.
>>
>> We can use the existing datastructures and defines used for
>> ETHTOOL_GRX* for new ethtool commands that do filtering.
>>
>> NIU can filter on these tuples too.
>
> Are you suggesting to extend the flow hash stuff that is already in
> ethtool (same ioctl)? If so, that makes sense. I'll go ahead and get
> some patches together for review.
Not exactly.
Rather, I'm saying to use a new ethtool command, but make use of the
existing flow hash datastructures as much as possible.
^ permalink raw reply
* [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 6:43 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Stephen Hemminger, David S. Miller, netdev, xiaosuo
don't go back.
don't go back.
----
net/sched/act_mirred.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b9aaab4..e1d8e2c 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -160,17 +160,7 @@ static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
if (net_ratelimit())
printk("mirred to Houston: device %s is gone!\n",
dev->name);
-bad_mirred:
- if (skb2 != NULL)
- kfree_skb(skb2);
- m->tcf_qstats.overlimits++;
- m->tcf_bstats.bytes += qdisc_pkt_len(skb);
- m->tcf_bstats.packets++;
- spin_unlock(&m->tcf_lock);
- /* should we be asking for packet to be dropped?
- * may make sense for redirect case only
- */
- return TC_ACT_SHOT;
+ goto bad_mirred;
}
skb2 = skb_act_clone(skb, GFP_ATOMIC);
@@ -199,6 +189,18 @@ bad_mirred:
dev_queue_xmit(skb2);
spin_unlock(&m->tcf_lock);
return m->tcf_action;
+
+bad_mirred:
+ if (skb2 != NULL)
+ kfree_skb(skb2);
+ m->tcf_qstats.overlimits++;
+ m->tcf_bstats.bytes += qdisc_pkt_len(skb);
+ m->tcf_bstats.packets++;
+ spin_unlock(&m->tcf_lock);
+ /* should we be asking for packet to be dropped?
+ * may make sense for redirect case only
+ */
+ return TC_ACT_SHOT;
}
static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
^ permalink raw reply related
* Re: RFC: ethtool support for n-tuple filter programming
From: Peter P Waskiewicz Jr @ 2009-11-09 6:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20091108.223848.234795788.davem@davemloft.net>
On Sun, 2009-11-08 at 23:38 -0700, David Miller wrote:
> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Date: Sun, 08 Nov 2009 21:49:21 -0800
>
> > On Sat, 2009-11-07 at 21:27 -0700, David Miller wrote:
> >> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> >> Date: Fri, 06 Nov 2009 10:57:21 -0800
> >>
> >> > Any comments, thoughts, suggestions, or ideas are welcome.
> >>
> >> We can use the existing datastructures and defines used for
> >> ETHTOOL_GRX* for new ethtool commands that do filtering.
> >>
> >> NIU can filter on these tuples too.
> >
> > Are you suggesting to extend the flow hash stuff that is already in
> > ethtool (same ioctl)? If so, that makes sense. I'll go ahead and get
> > some patches together for review.
>
> Not exactly.
>
> Rather, I'm saying to use a new ethtool command, but make use of the
> existing flow hash datastructures as much as possible.
Ah ok. Makes even more sense.
Thanks David.
-PJ
^ permalink raw reply
* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-09 7:10 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911091647.29655.rusty@rustcorp.com.au>
On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
>> > > +{
>> > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
>> > > + sizeof(struct virtio_net_hdr) : 0;
>> > > + int i;
>> > > + mutex_lock(&n->dev.mutex);
>> > > + n->dev.acked_features = features;
>> >
>> > Why is this called "acked_features"? Not just "features"? I expected
>> > to see code which exposed these back to userspace, and didn't.
>>
>> Not sure how do you mean. Userspace sets them, why
>> does it want to get them exposed back?
>
> There's something about the 'acked' which rubs me the wrong way.
> "enabled_features" is perhaps a better term than "acked_features"; "acked"
> seems more a user point-of-view, "enabled" seems more driver POV?
>
Hmm. Are you happy with the ioctl name? If yes I think being consistent
with that is important.
> set_features matches your ioctl names, but it sounds like a fn name :(
>
> It's marginal. And 'features' is shorter than both.
I started with this but I was always getting confused whether this
includes all features or just acked features. I'll go with
enabled_features.
>
>> > > + switch (ioctl) {
>> > > + case VHOST_SET_VRING_NUM:
>> >
>> > I haven't looked at your userspace implementation, but does a generic
>> > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
>> > sense? It'd be simpler here,
>>
>> Not by much though, right?
>>
>> > but not sure if it'd be simpler to use?
>>
>> The problem is with VHOST_SET_VRING_BASE as well. I want it to be
>> separate because I want to make it possible to relocate e.g. used ring
>> to another address while ring is running. This would be a good debugging
>> tool (you look at kernel's used ring, check descriptor, then update
>> guest's used ring) and also possibly an extra way to do migration. And
>> it's nicer to have vring size separate as well, because it is
>> initialized by host and never changed, right?
>
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume.
Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a
cached value for notify on empty, so what this does is clear the value.
What exactly do you refer to when you say "this looks wrong"?
This could trigger an extra notification if I ever called
trigger_irq without get first. As I don't, it in fact has no effect.
> The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
Yes, I remember that patch. There seems to be little point though, at
this stage.
>
> I would drop avail_idx altogether: get_user is basically free, and simplifies
> a lot. As most state is in the ring, all you need is an ioctl to save/restore
> the last_avail_idx.
avail_idx is there for notify on empty: I had this thought that it's
better to leave the avail cache line alone when we are triggering
interrupt to avoid bouncing it around if guest is updating it meanwhile
on another CPU, and I think my testing showed that it helped
performance, but could be a mistake. You don't believe this can help?
>
>> We could merge DESC, AVAIL, USED, and it will reduce the amount of code
>> in userspace. With both base, size and fds separate, it seemed a bit
>> more symmetrical to have desc/avail/used separate as well.
>> What's your opinion?
>
> Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.
Will do.
>
>> > For future reference, this is *exactly* the kind of thing which would have
>> > been nice as a followup patch. Easy to separate, easy to review, not critical
>> > to the core.
>>
>> Yes. It's not too late to split it out though: should I do it yet?
>
> Only if you're feeling enthused. It's lightly reviewed now.
Not really :) I'll keep this in mind for the future.
Thanks!
>
> Cheers,
> Rusty.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 7:17 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, xiaosuo
check the return value of ndo_select_queue()
Check the return value of ndo_select_queue(). If the value isn't smaller
than the real_num_tx_queues, print a warning message, and reset it to zero.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
net/core/dev.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b8f74cf..edf5ea6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
const struct net_device_ops *ops = dev->netdev_ops;
u16 queue_index = 0;
- if (ops->ndo_select_queue)
+ if (ops->ndo_select_queue) {
queue_index = ops->ndo_select_queue(dev, skb);
- else if (dev->real_num_tx_queues > 1)
+ if (queue_index >= dev->real_num_tx_queues) {
+ printk(KERN_WARNING "%s selects TX queue %d, "
+ "but real number of TX queues is %d\n",
+ dev->name, queue_index, dev->real_num_tx_queues);
+ queue_index = 0;
+ }
+ } else if (dev->real_num_tx_queues > 1) {
queue_index = skb_tx_hash(dev, skb);
+ }
skb_set_queue_mapping(skb, queue_index);
return netdev_get_tx_queue(dev, queue_index);
^ permalink raw reply related
* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-09 7:20 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911091647.29655.rusty@rustcorp.com.au>
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote:
> On Sun, 8 Nov 2009 10:05:16 pm Michael S. Tsirkin wrote:
> > On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> > > > +/* Caller must have TX VQ lock */
> > > > +static void tx_poll_stop(struct vhost_net *net)
> > > > +{
> > > > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > > > + return;
> > >
> > > likely? Really?
> >
> > Hmm ... yes. tx poll stop is called on each packet (as long as we do not
> > fill up 1/2 backend queue), the first call will stop polling
> > the rest checks state and does nothing.
> >
> > This is because we normally do not care when the message has left the
> > queue in backend device: we tell backend to send it and forget. We only
> > start polling when backend tx queue fills up.
>
> OK, good.
>
> > > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> > > > +{
> > > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > > > + sizeof(struct virtio_net_hdr) : 0;
> > > > + int i;
> > > > + mutex_lock(&n->dev.mutex);
> > > > + n->dev.acked_features = features;
> > >
> > > Why is this called "acked_features"? Not just "features"? I expected
> > > to see code which exposed these back to userspace, and didn't.
> >
> > Not sure how do you mean. Userspace sets them, why
> > does it want to get them exposed back?
>
> There's something about the 'acked' which rubs me the wrong way.
> "enabled_features" is perhaps a better term than "acked_features"; "acked"
> seems more a user point-of-view, "enabled" seems more driver POV?
>
> set_features matches your ioctl names, but it sounds like a fn name :(
Hmm. Are you happy with the ioctl name? If yes I think being consistent
with that is important.
> It's marginal. And 'features' is shorter than both.
I started with this but I was always getting confused whether this
includes all features or just acked features. I'll go with
enabled_features.
> > > > + switch (ioctl) {
> > > > + case VHOST_SET_VRING_NUM:
> > >
> > > I haven't looked at your userspace implementation, but does a generic
> > > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> > > sense? It'd be simpler here,
> >
> > Not by much though, right?
> >
> > > but not sure if it'd be simpler to use?
> >
> > The problem is with VHOST_SET_VRING_BASE as well. I want it to be
> > separate because I want to make it possible to relocate e.g. used ring
> > to another address while ring is running. This would be a good debugging
> > tool (you look at kernel's used ring, check descriptor, then update
> > guest's used ring) and also possibly an extra way to do migration. And
> > it's nicer to have vring size separate as well, because it is
> > initialized by host and never changed, right?
>
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume.
Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a
cached value for notify on empty, so what this does is clear the value.
What exactly do you refer to when you say "this looks wrong"?
This could trigger an extra notification if I ever called
trigger_irq without get first. As I don't, it in fact has no effect.
> The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
Yes, I remember that patch. There seems to be little point though, at
this stage.
>
> I would drop avail_idx altogether: get_user is basically free, and
> simplifies a lot. As most state is in the ring, all you need is an
> ioctl to save/restore the last_avail_idx.
avail_idx is there for notify on empty: I had this thought that it's
better to leave the avail cache line alone when we are triggering
interrupt to avoid bouncing it around if guest is updating it meanwhile
on another CPU, and I think my testing showed that it helped
performance, but could be a mistake. You don't believe this can help?
> > We could merge DESC, AVAIL, USED, and it will reduce the amount of code
> > in userspace. With both base, size and fds separate, it seemed a bit
> > more symmetrical to have desc/avail/used separate as well.
> > What's your opinion?
>
> Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.
OK, I'll do this.
> > > For future reference, this is *exactly* the kind of thing which would have
> > > been nice as a followup patch. Easy to separate, easy to review, not critical
> > > to the core.
> >
> > Yes. It's not too late to split it out though: should I do it yet?
>
> Only if you're feeling enthused. It's lightly reviewed now.
Not really :) I'll keep this in mind for the future.
Thanks!
> Cheers,
> Rusty.
>
> virtio: put last_used and last_avail index into ring itself.
>
> Generally, the other end of the virtio ring doesn't need to see where
> you're up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, if you want to save and restore a virtio_ring, but you're
> not the consumer because the kernel is using it directly.
>
> Fortunately, we have room to expand: the ring is always a whole number
> of pages and there's hundreds of bytes of padding after the avail ring
> and the used ring, whatever the number of descriptors (which must be a
> power of 2).
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/virtio/virtio_ring.c | 23 +++++++++++++++--------
> include/linux/virtio_ring.h | 12 +++++++++++-
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -71,9 +71,6 @@ struct vring_virtqueue
> /* Number we've added since last sync. */
> unsigned int num_added;
>
> - /* Last used index we've seen. */
> - u16 last_used_idx;
> -
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> @@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
>
> static inline bool more_used(const struct vring_virtqueue *vq)
> {
> - return vq->last_used_idx != vq->vring.used->idx;
> + return vring_last_used(&vq->vring) != vq->vring.used->idx;
> }
>
> static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_used_elem *u;
> void *ret;
> unsigned int i;
>
> @@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
> return NULL;
> }
>
> - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> + u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
> + i = u->id;
> + *len = u->len;
> + /* Make sure we don't reload i after doing checks. */
> + rmb();
>
> if (unlikely(i >= vq->vring.num)) {
> BAD_RING(vq, "id %u out of range\n", i);
> @@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
> /* detach_buf clears data, so grab it now. */
> ret = vq->data[i];
> detach_buf(vq, i);
> - vq->last_used_idx++;
> + vring_last_used(&vq->vring)++;
> +
> END_USE(vq);
> return ret;
> }
> @@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
> vq->vq.name = name;
> vq->notify = notify;
> vq->broken = false;
> - vq->last_used_idx = 0;
> vq->num_added = 0;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> @@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
>
> + /* We publish indices whether they offer it or not: if not, it's junk
> + * space anyway. But calling this acknowledges the feature. */
> + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
> +
> /* No callback? Tell other side not to bother us. */
> if (!callback)
> vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> @@ -443,6 +448,8 @@ void vring_transport_features(struct vir
> switch (i) {
> case VIRTIO_RING_F_INDIRECT_DESC:
> break;
> + case VIRTIO_RING_F_PUBLISH_INDICES:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -29,6 +29,9 @@
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> +/* We publish our last-seen used index at the end of the avail ring. */
> +#define VIRTIO_RING_F_PUBLISH_INDICES 29
> +
> /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
> struct vring_desc
> {
> @@ -87,6 +90,7 @@ struct vring {
> * __u16 avail_flags;
> * __u16 avail_idx;
> * __u16 available[num];
> + * __u16 last_used_idx;
> *
> * // Padding to the next align boundary.
> * char pad[];
> @@ -95,6 +99,7 @@ struct vring {
> * __u16 used_flags;
> * __u16 used_idx;
> * struct vring_used_elem used[num];
> + * __u16 last_avail_idx;
> * };
> */
> static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> @@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
> {
> return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
> + align - 1) & ~(align - 1))
> - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
> + + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
> }
>
> +/* We publish the last-seen used index at the end of the available ring, and
> + * vice-versa. These are at the end for backwards compatibility. */
> +#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
> +#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
> +
> #ifdef __KERNEL__
> #include <linux/irqreturn.h>
> struct virtio_device;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: jamal @ 2009-11-09 7:22 UTC (permalink / raw)
To: xiaosuo; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <4AF7B9F4.6010507@gmail.com>
On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
> don't go back.
I didnt follow the motivation for the patch. Is it a bug
fix for some setup? I may be too jet-lagged and i am missing
the functional difference vs what was already there.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 7:26 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <4AF7C1FD.8000302@gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 09 Nov 2009 15:17:17 +0800
> check the return value of ndo_select_queue()
>
> Check the return value of ndo_select_queue(). If the value isn't smaller
> than the real_num_tx_queues, print a warning message, and reset it to zero.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
Make it a WARN() so that it ends up in kerneloops.org
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 8:31 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257751376.8009.2.camel@bigi>
On Mon, Nov 9, 2009 at 3:22 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
>> don't go back.
>
> I didnt follow the motivation for the patch. Is it a bug
> fix for some setup? I may be too jet-lagged and i am missing
> the functional difference vs what was already there.
>
code cleanup! :)
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 8:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20091108.232647.47855733.davem@davemloft.net>
On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
> From: Changli Gao <xiaosuo@gmail.com>
> Date: Mon, 09 Nov 2009 15:17:17 +0800
>
>> check the return value of ndo_select_queue()
>>
>> Check the return value of ndo_select_queue(). If the value isn't smaller
>> than the real_num_tx_queues, print a warning message, and reset it to zero.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> Make it a WARN() so that it ends up in kerneloops.org
>
Like this?
WARN(1, "%s selects TX queue %d, "
"but real number of TX queues is %d\n",
dev->name, queue_index, dev->real_num_tx_queues);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 9:13 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <412e6f7f0911090044v33c331edg6096c0f9b9db4143@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 9 Nov 2009 16:44:05 +0800
> On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
>> From: Changli Gao <xiaosuo@gmail.com>
>> Date: Mon, 09 Nov 2009 15:17:17 +0800
>>
>>> check the return value of ndo_select_queue()
>>>
>>> Check the return value of ndo_select_queue(). If the value isn't smaller
>>> than the real_num_tx_queues, print a warning message, and reset it to zero.
>>>
>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>
>> Make it a WARN() so that it ends up in kerneloops.org
>>
> Like this?
> WARN(1, "%s selects TX queue %d, "
> "but real number of TX queues is %d\n",
> dev->name, queue_index, dev->real_num_tx_queues);
Yes, something like that.
^ permalink raw reply
* [PATCH] tcp: provide more information on the tcp receive_queue bugs
From: Ilpo Järvinen @ 2009-11-09 9:19 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Arjan van de Ven, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2300 bytes --]
The addition of rcv_nxt allows to discern whether the skb
was out of place or tp->copied. Also catch fancy combination
of flags if necessary (sadly we might miss the actual causer
flags as it might have already returned).
Btw, we perhaps would want to forward copied_seq in
somewhere or otherwise we might have some nice loop with
WARN stuff within but where to do that safely I don't
know at this stage until more is known (but it is not
made significantly worse by this patch).
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
net/ipv4/tcp.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 98440ad..f1813bc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1183,7 +1183,9 @@ void tcp_cleanup_rbuf(struct sock *sk, int copied)
#if TCP_DEBUG
struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
- WARN_ON(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq));
+ WARN(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq),
+ KERN_INFO "cleanup rbuf bug: copied %X seq %X rcvnxt %X\n",
+ tp->copied_seq, TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt);
#endif
if (inet_csk_ack_scheduled(sk)) {
@@ -1430,11 +1432,13 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
/* Now that we have two receive queues this
* shouldn't happen.
*/
- if (before(*seq, TCP_SKB_CB(skb)->seq)) {
- printk(KERN_INFO "recvmsg bug: copied %X "
- "seq %X\n", *seq, TCP_SKB_CB(skb)->seq);
+ if (WARN(before(*seq, TCP_SKB_CB(skb)->seq),
+ KERN_INFO "recvmsg bug: copied %X "
+ "seq %X rcvnxt %X fl %X\n", *seq,
+ TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
+ flags))
break;
- }
+
offset = *seq - TCP_SKB_CB(skb)->seq;
if (tcp_hdr(skb)->syn)
offset--;
@@ -1443,8 +1447,9 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
if (tcp_hdr(skb)->fin)
goto found_fin_ok;
WARN(!(flags & MSG_PEEK), KERN_INFO "recvmsg bug 2: "
- "copied %X seq %X\n", *seq,
- TCP_SKB_CB(skb)->seq);
+ "copied %X seq %X rcvnxt %X fl %X\n",
+ *seq, TCP_SKB_CB(skb)->seq,
+ tp->rcv_nxt, flags);
}
/* Well, if we have backlog, try to process it now yet. */
--
1.5.6.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox