public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Ed L Cashin <ecashin@coraid.com>
To: Andi Kleen <ak@muc.de>
Cc: Jens Axboe <axboe@suse.de>,
	Linux Kernel List <linux-kernel@vger.kernel.org>,
	jgarzik@pobox.com
Subject: Re: [BUG] ATA over Ethernet __init calling __exit
Date: Thu, 13 Jan 2005 15:52:05 -0500	[thread overview]
Message-ID: <87is616oi2.fsf@coraid.com> (raw)
In-Reply-To: m1wtuh2kah.fsf@muc.de

Andi Kleen <ak@muc.de> writes:

...
> In general I think it was a bad idea to merge this driver at all.
> The protocol is obviously broken by design - they use a 16 bit sequence
> number space which has been proven for many years (in ip fragmentation)
> to be far too small for modern network performance.

While that experience may apply well to IP, this is a non-IP protocol
for a single LAN.  For any given AoE device, there are only a few
outstanding packets at any given time.

For existing AoE devices that number of outstanding packets is only
three!  So, with only three packets on the wire at any time for a
given device, 16 bits is overkill.  In fact, the AoE protocol allows
the AoE device to specify how many outstanding packets it supports.
That number is only 16 bits wide.  

If it ever did become desirable, we could use a couple more bits for
the sequence number by borrowing from the low bits of jiffies that we
use to estimate the RTT, but it doesn't seem likely to ever be
desirable.

> Also the memory allocation without preallocation in the block write
> out path looks quite broken too and will most likely will lead to deadlocks
> under high load.
>
> (I wrote a detailed review when it was posted but apparently it 
> disappeared or I never got any answer at least) 

I think you're talking about your suggestion that the skb allocation
could lead to a deadlock.  If I'm correct, this issue is similar to
the one that led us to create a mempool for the buf structs we use.

> IMHO this thing should have never been merged in this form. Can it 
> still be backed out?

The sequence number is a non-issue, and for the deadlock danger you
originally suggested that we add a warning against using aoe storage
for swap.  We could add such a warning to the driver's documentation
and start working on a patch that avoids deadlocking on the skb
allocation.  There's no need to panic and force users go get a third
party driver.

-- 
  Ed L Cashin <ecashin@coraid.com>


  reply	other threads:[~2005-01-13 20:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-01-13  0:09 [BUG] ATA over Ethernet __init calling __exit Russell King
2005-01-13  8:50 ` Jens Axboe
2005-01-13 19:36   ` Andi Kleen
2005-01-13 20:52     ` Ed L Cashin [this message]
2005-01-13 21:53       ` Andi Kleen
2005-01-13 22:09         ` Alan Cox
2005-01-13 22:12         ` Ed L Cashin
2005-01-13 22:48           ` Andi Kleen
2005-01-13 22:59             ` Ed L Cashin
2005-01-13 17:35 ` [PATCH] " Ed L Cashin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87is616oi2.fsf@coraid.com \
    --to=ecashin@coraid.com \
    --cc=ak@muc.de \
    --cc=axboe@suse.de \
    --cc=jgarzik@pobox.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox