From owner-freebsd-current@FreeBSD.ORG  Fri Jul 26 15:03:48 2013
Return-Path: <owner-freebsd-current@FreeBSD.ORG>
Delivered-To: freebsd-current@FreeBSD.org
Received: from mx1.freebsd.org (mx1.freebsd.org
 [IPv6:2001:1900:2254:206a::19:1])
 (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits))
 (No client certificate requested)
 by hub.freebsd.org (Postfix) with ESMTP id 05A8EB63
 for <freebsd-current@FreeBSD.org>; Fri, 26 Jul 2013 15:03:48 +0000 (UTC)
 (envelope-from luigi@onelab2.iet.unipi.it)
Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238])
 by mx1.freebsd.org (Postfix) with ESMTP id BEFCA2037
 for <freebsd-current@FreeBSD.org>; Fri, 26 Jul 2013 15:03:47 +0000 (UTC)
Received: by onelab2.iet.unipi.it (Postfix, from userid 275)
 id 3A8517300A; Fri, 26 Jul 2013 17:01:32 +0200 (CEST)
Date: Fri, 26 Jul 2013 17:01:32 +0200
From: Luigi Rizzo <rizzo@iet.unipi.it>
To: FreeBSD Current <freebsd-current@FreeBSD.org>
Subject: any need to "undo" a selrecord() ?
Message-ID: <20130726150132.GA74293@onelab2.iet.unipi.it>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.5.20 (2009-06-14)
X-BeenThere: freebsd-current@freebsd.org
X-Mailman-Version: 2.1.14
Precedence: list
List-Id: Discussions about the use of FreeBSD-current
 <freebsd-current.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/freebsd-current>, 
 <mailto:freebsd-current-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-current>
List-Post: <mailto:freebsd-current@freebsd.org>
List-Help: <mailto:freebsd-current-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
 <mailto:freebsd-current-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Fri, 26 Jul 2013 15:03:48 -0000

hi,
in an attempt to simplify the locking in netmap,
i would like to code the poll handler along these lines

    netmap_poll() {
        ...

        /* PART1 */

        revents = 0;
        /* see if any of the rings has data */
        for (i = first_ring; i < last_ring; i++) {
            na->nm_txsync(ifp, i, 1);   /* check for space      */
            if (kring->ring->avail > 0)
                revents |= POLLOUT;
        }
        if (!(revents & POLLOUT)) {
            selrecord(td, main_wq);     /* apparently not, want wakeup */
        }

        /* PART2 */

        /* but then retry in case someone new data comes in */
        for (i = first_ring; i < last_ring; i++) {
            na->nm_txsync(ifp, i, 1);   /* check for space      */
            if (kring->ring->avail > 0)
                revents |= POLLOUT;
        }
        ...
   }


The tx interrupt service routine would only do a

    *_tx_interrupt() {
        ...
        selwakeuppri(main_wq, PI_NET);
        ...
    }

 
The existing code does not have the PART2 block, and wraps both
"PART1" and the selwakeuppri() in a lock/unlock pair, so there is
no race between the txsync()/test/selrecord() and selwakeup().

But the nesting of locks (*_txsync() has its own lock) complicates
the code a lot, especially when attaching NICs and VALE switches,
not to mention that PART1 can be fairly time consuming so I don't want
other threads to spin waiting for it to finish.

Hence i am willing to pay the cost of a retry phase
and remove the "external" lock.

My concern is whether i should do something to "undo" the
selrecord() in case i find that some queue is available in
the retry phase. Probably not (since the same problem would
occur if another file descriptor becomes ready),
but just wanted to be sure.

	cheers
	luigi