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 Fri, Jun 20, 2003 at 11:43:39PM -0600, Lou Langholtz wrote:
    >
    >In addition to comments from Andrew:
    >. . .
    >
    >
    >>+static nbd_device_t nbd_devs[MAX_NBD];
    >>+static struct request_queue nbd_queue[MAX_NBD];
    >>+static spinlock_t nbd_lock[MAX_NBD];
    >>
    >>
    >
    >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???

    >>+static uint32_t request_magic;
    >>
    >>
    >
    >??? htonl(NBD_REQUEST_MAGIC) is perfectly OK in the place where you
    >use it and more likely than not will give better code.
    >
    >
    >
    >>+static uint32_t reply_magic;
    >>
    >>
    >
    >Ditto.
    >
    What's wrong with having an explicit cache of this value that we can
    rest assured doesn't in the worst case get compiled into multiple calls
    to the htonl code?? Possible waste of one 4 byte memory location in the
    worst compiler case or is there another problem?

    >. . .
    >
    >
    >>+static void __init init_nbd_dev(nbd_device_t *dev)
    >>+{
    >>+#ifdef PARANOIA
    >>+ dev->magic = LO_MAGIC;
    >>+#endif
    >>+ atomic_set(&dev->refcnt, 0);
    >>+ dev->flags = 0;
    >>+ spin_lock_init(&dev->lock);
    >>+ dev->harderror = 0;
    >>+ nbd_qsys_init(&dev->tx_queue);
    >>+ nbd_qsys_init(&dev->rx_queue);
    >>+ dev->file = NULL;
    >>+ dev->sock = NULL;
    >>+ memset(&dev->sin, 0, sizeof(dev->sin));
    >>+ dev->errcnt = 0;
    >>+ dev->lasterr = ENOTCONN;
    >>+ dev->closed = (RCV_SHUTDOWN|SEND_SHUTDOWN);
    >>+ dev->ss_thread.task = NULL;
    >>+ dev->tx_thread.task = NULL;
    >>+ dev->rx_thread.task = NULL;
    >>+ atomic_set(&dev->num_io_threads, 0);
    >>
    >>
    >
    >*Ugh*. These suckers are already zero-filled.
    >
    >
    Yeah. I'm a bit to attracted perhaps to zero filling things explicitly
    in my coding style. Sigh.

    >>+ requests_out = 0;
    >>+ qhandler_loops = 0;
    >>
    >>
    >
    >Static variables that do not have explicit initializer are initialized with 0.
    >
    >
    Ditto.

    >>+/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
    >>+ *
    >>+ * End of definitions shared with user space.
    >>+ * From here on out, these definitions are only for kernel (driver).
    >>+ */
    >>+
    >>+#ifdef MAJOR_NR
    >>
    >>
    >
    >a) that's ifdef __KERNEL__
    >b) use separate headers.
    >
    >
    Historically, some code used MAJOR_NR this way (like the original nbd
    driver unfortunately), and I didn't change that. Seperating the header
    was something I'd thought about but in the end felt that just changing
    two existing files was bad enough without going off and adding a third
    or more files. It's worth reconsidering though also. Thanks!

    -
    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.046 / U:0.268 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site