This patch (as964) was suggested by Steffen Koepf. It makes
usb_get_descriptor() retry on all errors other than ETIMEDOUT, instead
of only on EPIPE. This helps with some devices.
Signed-off-by: Alan Stern <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/usb/core/message.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
Index: 2.6/drivers/usb/core/message.c
===================================================================
--- 2.6.orig/drivers/usb/core/message.c
+++ 2.6/drivers/usb/core/message.c
@@ -608,12 +608,12 @@ int usb_get_descriptor(struct usb_device
memset(buf,0,size); // Make sure we parse really received data
for (i = 0; i < 3; ++i) {
- /* retry on length 0 or stall; some devices are flakey */
+ /* retry on length 0 or error; some devices are flakey */
result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
(type << 8) + index, 0, buf, size,
USB_CTRL_GET_TIMEOUT);
- if (result == 0 || result == -EPIPE)
+ if (result <= 0 && result != -ETIMEDOUT)
continue;
if (result > 1 && ((u8 *)buf)[1] != type) {
result = -EPROTO;
--
On Sat, 13 Oct 2007, Willy Tarreau wrote:
> It's possible that new SACK blocks that should trigger new LOST
> markings arrive with new data (which previously made is_dupack
> false). In addition, I think this fixes a case where we get
> a cumulative ACK with enough SACK blocks to trigger the fast
> recovery (is_dupack would be false there too).
>
> I'm not completely pleased with this solution because readability
> of the code is somewhat questionable as 'is_dupack' in SACK case
> is no longer about dupacks only but would mean something like
> 'lost_marker_work_todo' too... But because of Eifel stuff done
> in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> the if statement which seems attractive solution. Nevertheless,
> I didn't like adding another variable just for that either... :-)
>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> net/ipv4/tcp_input.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> Index: 2.6/net/ipv4/tcp_input.c
> ===================================================================
> --- 2.6.orig/net/ipv4/tcp_input.c
> +++ 2.6/net/ipv4/tcp_input.c
> @@ -1951,7 +1951,10 @@ tcp_fastretrans_alert(struct sock *sk, u
> {
> struct inet_connection_sock *icsk = inet_csk(sk);
> struct tcp_sock *tp = tcp_sk(sk);
> - int is_dupack = (tp->snd_una == prior_snd_una && !(flag&FLAG_NOT_DUP));
> + int is_dupack = (tp->snd_una == prior_snd_una &&
> + (!(flag&FLAG_NOT_DUP) ||
> + ((flag&FLAG_DATA_SACKED) &&
> + (tp->fackets_out > tp->reordering))));
>
> /* Some technical things:
> * 1. Reno does not count dupacks (sacked_out) automatically. */
FYI,
This ended up being a non complete fix. Day after these two patches
(11-12) I submitted two other patches to complete this fix series (got
bitten by release-early-release-often, fixed day-after-submission
thoughts in those two later patches). For some reason these two keep
floating around as separate ones from those two later ones.
To make things even more complicated, eb7bdad82e8 (see stable-2.6.22)
could have been split more logically to do_lost addition and
FLAG_SND_UNA_ADVANCED parts (but that didn't occur to me back then).
All of them listed here (from stable-2.6.22 since one of them is
reduced from mainline version):
6d742fb6e2b8913457e1282e1be77d6f4e45af00 Fix TCP DSACK cwnd handling
eb7bdad82e8af48e1ed1b650268dc85ca7e9ff39 Handle snd_una in tcp_cwnd_down()
8385cffd22359ad561a173accefeb354bd606ce4 TCP: Fix TCP handling of SACK in
783366ad4b212cde069c50903494eb6a6b83958c TCP: Fix TCP rate-halving on
--
i.
Hi Ilpo,
On Sat, Oct 13, 2007 at 08:15:52PM +0300, Ilpo J?rvinen wrote:
> On Sat, 13 Oct 2007, Willy Tarreau wrote:
>
> > It's possible that new SACK blocks that should trigger new LOST
> > markings arrive with new data (which previously made is_dupack
> > false). In addition, I think this fixes a case where we get
> > a cumulative ACK with enough SACK blocks to trigger the fast
> > recovery (is_dupack would be false there too).
> >
> > I'm not completely pleased with this solution because readability
> > of the code is somewhat questionable as 'is_dupack' in SACK case
> > is no longer about dupacks only but would mean something like
> > 'lost_marker_work_todo' too... But because of Eifel stuff done
> > in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> > the if statement which seems attractive solution. Nevertheless,
> > I didn't like adding another variable just for that either... :-)
> >
> > Signed-off-by: Ilpo Järvinen <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > net/ipv4/tcp_input.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > Index: 2.6/net/ipv4/tcp_input.c
> > ===================================================================
> > --- 2.6.orig/net/ipv4/tcp_input.c
> > +++ 2.6/net/ipv4/tcp_input.c
> > @@ -1951,7 +1951,10 @@ tcp_fastretrans_alert(struct sock *sk, u
> > {
> > struct inet_connection_sock *icsk = inet_csk(sk);
> > struct tcp_sock *tp = tcp_sk(sk);
> > - int is_dupack = (tp->snd_una == prior_snd_una && !(flag&FLAG_NOT_DUP));
> > + int is_dupack = (tp->snd_una == prior_snd_una &&
> > + (!(flag&FLAG_NOT_DUP) ||
> > + ((flag&FLAG_DATA_SACKED) &&
> > + (tp->fackets_out > tp->reordering))));
> >
> > /* Some technical things:
> > * 1. Reno does not count dupacks (sacked_out) automatically. */
>
> FYI,
>
> This ended up being a non complete fix. Day after these two patches
> (11-12) I submitted two other patches to complete this fix series (got
> bitten by release-early-release-often, fixed day-after-submission
> thoughts in those two later patches). For some reason these two keep
> floating around as separate ones from those two later ones.
>
> To make things even more complicated, eb7bdad82e8 (see stable-2.6.22)
> could have been split more logically to do_lost addition and
> FLAG_SND_UNA_ADVANCED parts (but that didn't occur to me back then).
>
> All of them listed here (from stable-2.6.22 since one of them is
> reduced from mainline version):
>
> 6d742fb6e2b8913457e1282e1be77d6f4e45af00 Fix TCP DSACK cwnd handling
> eb7bdad82e8af48e1ed1b650268dc85ca7e9ff39 Handle snd_una in tcp_cwnd_down()
> 8385cffd22359ad561a173accefeb354bd606ce4 TCP: Fix TCP handling of SACK in
> 783366ad4b212cde069c50903494eb6a6b83958c TCP: Fix TCP rate-halving on
Thanks for your help, I really appreciate it. In fact, I've reviewed them
four, but two of them did not apply and the code looked somewhat different,
so I considered them irrelevant to 2.6.20. I didn't understand that they
were all related, so maybe I checked them in a wrong order.
I'll recheck all that in the right sequence and will merge them four, or
get back to you if something still puzzles me.
Thanks!
Willy
On Sat, Oct 13, 2007 at 07:22:14PM +0200, Willy Tarreau wrote:
>...
> Thanks for your help, I really appreciate it. In fact, I've reviewed them
> four, but two of them did not apply and the code looked somewhat different,
> so I considered them irrelevant to 2.6.20. I didn't understand that they
> were all related, so maybe I checked them in a wrong order.
>
> I'll recheck all that in the right sequence and will merge them four, or
> get back to you if something still puzzles me.
I discussed this issue with Ilpo just yesterday regarding 2.6.16, and
the result of our discussion was that I reverted it.
TCP being in some situations a bit more conservative than it should be
isn't a big issue and not worth backporting with a risk of introducing
a regression.
I'd recommend you simply drop the two patches for 2.6.20.
> Thanks!
> Willy
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
Hi Adrian,
On Sat, Oct 13, 2007 at 07:50:36PM +0200, Adrian Bunk wrote:
> On Sat, Oct 13, 2007 at 07:22:14PM +0200, Willy Tarreau wrote:
> >...
> > Thanks for your help, I really appreciate it. In fact, I've reviewed them
> > four, but two of them did not apply and the code looked somewhat different,
> > so I considered them irrelevant to 2.6.20. I didn't understand that they
> > were all related, so maybe I checked them in a wrong order.
> >
> > I'll recheck all that in the right sequence and will merge them four, or
> > get back to you if something still puzzles me.
>
> I discussed this issue with Ilpo just yesterday regarding 2.6.16, and
> the result of our discussion was that I reverted it.
OK.
> TCP being in some situations a bit more conservative than it should be
> isn't a big issue and not worth backporting with a risk of introducing
> a regression.
I agree with this. The impression I got from the description of the two
patches I merged was that the problems they fix were quite annoying. But
maybe I should take that with a grain of salt.
> I'd recommend you simply drop the two patches for 2.6.20.
That sounds OK to me. If 2.6.16 is fine without the patches, 2.6.20
certainly is, particularly if we keep in mind that it's a last version.
Thanks very much for your insights Adrian,
Willy
On Sat, 13 Oct 2007, Willy Tarreau wrote:
> On Sat, Oct 13, 2007 at 07:50:36PM +0200, Adrian Bunk wrote:
> > On Sat, Oct 13, 2007 at 07:22:14PM +0200, Willy Tarreau wrote:
> > >...
> > > Thanks for your help, I really appreciate it. In fact, I've reviewed them
> > > four, but two of them did not apply and the code looked somewhat different,
> > > so I considered them irrelevant to 2.6.20. I didn't understand that they
> > > were all related, so maybe I checked them in a wrong order.
> > >
> > > I'll recheck all that in the right sequence and will merge them four, or
> > > get back to you if something still puzzles me.
> >
> > I discussed this issue with Ilpo just yesterday regarding 2.6.16, and
> > the result of our discussion was that I reverted it.
>
> OK.
>
> > TCP being in some situations a bit more conservative than it should be
This of course understatement from what I wrote about the performance:
"I would rather put it this way: Without those four patches TCP
just is very much more conservative than with them but still
works. " Emphasis on word very. Yet this isn't a correctness issue.
> > isn't a big issue and not worth backporting with a risk of introducing
> > a regression.
I agree that it's not that big issue due to the cases that are affected.
The flow must simulataneously be bidirectional and get at least one of the
directions to suffer from congestion. Considering that this problem has
been there very long (definately predates 2.6 series, probably it has
occured since ratehalving was added, I don't know when), and nobody has
complained because of poor performance, I'd claim it's highly unlikely
they will, in the near future... :-)
> I agree with this. The impression I got from the description of the two
> patches I merged was that the problems they fix were quite annoying. But
> maybe I should take that with a grain of salt.
No, it's not a grain of salt. I would say its utterly broken, out loud.
But many people are not that much into time-seq graphs (that I'm familiar
with), they are pleased when it seems to work well enough even though
from my perspective, it is simply unacceptable in worst cases (not
speaking of theoretical ones here, have seen very bad performance). Not
that it always is that bad, depends on phase of the opposite direction
what happens.
Somebody asked me when those four patches were made about this, I put
these there back then:
http://www.cs.helsinki.fi/u/ijjarvin/bidir-showcase/
They are generated from my old test archives and thus may have a bit
differences in TCP variant, which may slightly differ from mainline
here and there (my point was just to show how it breaks). Typical
people wouldn't even notice those minor differences compared with the
bidir brokeness which is very visible in all except in the fixed "ok"
case. Please judge for yourself whether I overexaggrated or not... :-)
--
i.