lkml.org 
[lkml]   [2003]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH] nbd driver for 2.5.72
viro@parcelfarce.linux.theplanet.co.uk wrote:

>On Sat, Jun 21, 2003 at 10:39:12AM -0600, Lou Langholtz wrote:
>
>
>>>Why not put these into nbd_device?
>>>
>>>
>>>
>>I'd considered that and I'm reconsidering it again now. Not convinced
>>which way to go... Putting something as large as struct request_queue
>>within the nbd_device seems unbalanced somehow. Then again, until 2.5
>>the request_queue was typically shared by multiple devices of the same
>>MAJOR so part of the way the code is has to do with the legacy code.
>>Like the nbd_lock spinlock array and the struct request_queue queue_lock
>>field. Along the lines you're pushing for, why not have struct
>>requests_queue's queue_lock field then be the spinlock itself instead of
>>just being a pointer to a spinlock???
>>
>>
>
>Because often that lock protects driver-internal objects that are used
>by all queues.
>
Not sure I understand what you're saying... I was going by the kernel
blk_init_queue(q, rfn, lock) source that assigns q->queue_lock to the
given lock pointer. Given how big struct request_queue was compared to a
spinlock_t and since we require all disks to have there very own
seperate struct request_queue (by virtue of all the sysfs stuff imbedded
now in there), I'm pursauded to find requiring each request_queue to
have its very own lock (by making queue_lock a spinlock_t rather than a
pointer to such) a relatively low weighted addition for worthwhile gain.
I don't doubt that I've missed something though. So unless some more
experienced folks chime in for having queue_lock become a spinlock_t
instead of spinlock_t *, I'll just not say anymore about queue_lock.

>Prefered variant (actually, we'll have to do it in 2.5 anyway) is to
>allocate request_queue dynamically. Just put a pointer to it into nbd_device.
>
The best suggestion I've gotten so far was from Andrew Morton who
rightfully said this patch was enormous. His comment was that it should
be broken into separate logical patches which I'm working on now as the
highest priority. I'm about to send out a comparitively miniscule patch
that is meant as a start toward breaking this all down. It's probably
the most critical changes currently needed by nbd anyway just to get it
back to a usable state in 2.5 (for example try to rmmod nbd in 2.5.72
and earlier - the result corrupts memory). The reason I mention this is
because putting a pointer to the request_queue struct probably wouldn't
happen then for a while till I get through enough incremental patches.

>BTW, could you please kill the ..._t silliness? There is nothing wroung
>with using 'struct nbd_device' directly.
>
>
Will do. Personally, I like the _t silliness, but then I can see the
value enough either way and agree that consistancy tips any possible
balance. ;-)

>. . .
>htonl() honours constants. If it doesn't, we are in for much more serious
>problems, simply because a lot of codepaths in networking are using it.
>A lot. IOW, you are obfuscating code for no good reason (and add an extra
>memory access, thus giving actually worse code - it's not an optimisation
>at all).
>
>
Okay. I'll definately change this too. Thanks again for all your
feedback!!!!

-
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/

\
 
 \ /
  Last update: 2005-03-22 13:36    [W:0.055 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site