Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933304AbXEEQXH (ORCPT ); Sat, 5 May 2007 12:23:07 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S933298AbXEEQXG (ORCPT ); Sat, 5 May 2007 12:23:06 -0400 Received: from gw.goop.org ([64.81.55.164]:37205 "EHLO mail.goop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933287AbXEEQXE (ORCPT ); Sat, 5 May 2007 12:23:04 -0400 Message-ID: <463C56D3.8060609@goop.org> Date: Sat, 05 May 2007 03:05:07 -0700 From: Jeremy Fitzhardinge User-Agent: Thunderbird 1.5.0.10 (X11/20070302) MIME-Version: 1.0 To: Christoph Hellwig , Jeremy Fitzhardinge , Andi Kleen , Andrew Morton , virtualization@lists.osdl.org, lkml , Chris Wright , Ian Pratt , Christian Limpach , netdev@vger.kernel.org, Jeff Garzik , Stephen Hemminger , Rusty Russell , Herbert Xu , Keir Fraser Subject: Re: [patch 25/29] xen: Add the Xen virtual network device driver. References: <20070504232051.411946839@goop.org> <20070504232121.492190579@goop.org> <20070505091624.GA8890@infradead.org> In-Reply-To: <20070505091624.GA8890@infradead.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2415 Lines: 64 Christoph Hellwig wrote: > There only seems to be a module description but no actual paramter for > this. I wish people would have listened to me back then and made the > description part of the modular_param statement.. > Uh, what did I miss? Oh, I see, I need a module_param(rx_mode, int, 0600) or something? Or maybe with a callback if we can change it on the fly? And enum type? Or maybe I string? I'll sort it out. >> + >> +#define RX_COPY_THRESHOLD 256 >> + >> +#define GRANT_INVALID_REF 0 >> + >> +#define NET_TX_RING_SIZE __RING_SIZE((struct xen_netif_tx_sring *)0, PAGE_SIZE) >> +#define NET_RX_RING_SIZE __RING_SIZE((struct xen_netif_rx_sring *)0, PAGE_SIZE) >> > > __RING_SIZE is not in my tree, so it seems to be some kind of Xen > addition. Can you make that clear in the name and give it a less > awkware calling convention, e.g. only pass in the type, not a null > pointer of the given type? > Yeah. The Xen ring stuff is a bit full of magic macros, so I was going to look at inlineizing/re-namespacing it in a separate patch. >> +/* >> + * Implement our own carrier flag: the network stack's version causes delays >> + * when the carrier is re-enabled (in particular, dev_activate() may not >> + * immediately be called, which can cause packet loss). >> + */ >> +#define netfront_carrier_on(netif) ((netif)->carrier = 1) >> +#define netfront_carrier_off(netif) ((netif)->carrier = 0) >> +#define netfront_carrier_ok(netif) ((netif)->carrier) >> > > This doesn't implement my review suggestion despite you ACKing > them. Didn't you like it in the end or did you simply forget > about it? > Sorry, I forgot about it. I was waiting to hear back from network people about what this is actually for, and whether we really need it. Rusty said in his review: > Well, you only call netfront_carrier_on() from one place, so it's pretty > easy to do "netif_carrier_on(); dev_activate();" there. > > I don't think this is critical though. > It wasn't obvious to me whether this meant that we could avoid having a netfront-private carrier flag but still get quick response by using "netif_carrier_on(); dev_activate();". J - 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/