lkml.org 
[lkml]   [2007]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 7/9] lguest: the net driver
Rusty Russell wrote:
> Hi Jeff,
>
> Thanks for your review. Questions below.
>
> On Wed, 2007-05-09 at 08:28 -0400, Jeff Garzik wrote:
>> akpm@linux-foundation.org wrote:
>>> +static void transfer_packet(struct net_device *dev,
> ...
>>> + hcall(LHCALL_SEND_DMA, peer_key(info,peernum), __pa(&dma), 0);
>> __pa() should not be used in any driver.
>>
>> At the very least, lguest helper code should wrap this.
>
> I realize your continual battle with this, but adding a layer of
> indirection doesn't seem like it will add clarity. The issues with
> __pa() are reasonably known (don't hand it a vmalloc address, for
> example). Any wrapper I create would be another hurdle to jump 8(

You don't want this low level stuff in drivers.

All such details should be hidden away in the arch code, which in your
case is the lguest support code.

lguest should present a nice, friendly driver API that any Computer
Science freshman at university will understand. Because that's the
level at which we driver writers exist... :)


>>> +static irqreturn_t lguestnet_rcv(int irq, void *dev_id)
> ...
>>> + return done ? IRQ_HANDLED : IRQ_NONE;
>> Using NAPI would be preferable...
>
> I'm not so convinced: scheduling tends to give us pretty good interrupt
> mitigation. However, if you wish to send a patch, I'd be happy to
> benchmark the two 8)

NAPI means system-wide load leveling, across multiple network
interfaces. Lack of NAPI can mean competition at higher loads. Though
maybe that's less important with lguest.


>>> + /* Ethernet defaults with some changes */
>>> + ether_setup(dev);
>>> + dev->set_mac_address = NULL;
>> why NULL?
>
> Because it's not implemented: our MAC is advertised in the device page
> and we'd have to change it there too.
>
> Trivial to do, but is there a compelling reason to implement it?

Bonding, and situations where you /do/ want the MAC address to "leak"
out of the host onto the wider net.


>>> + dev->mem_start = ((unsigned long)desc->pfn << PAGE_SHIFT);
>>> + dev->mem_end = dev->mem_start + PAGE_SIZE * desc->num_pages;
>>> + dev->irq = lgdev->index+1;
>> don't fill in mem_start, mem_end and irq. they are useless, and for
>> lguest, misleading.
>
> You meant to type "useful and accurate", I think? They show up in
> ifconfig, so you can see what the underlying devices are using. They're
> as useful for virtual hardware as they are for physical hardware.

Indeed -- they are useless for physical hardware, as well.

Those items have not been used since the ISA days. Hardware is far more
complex than those three variables can provide, so the wise choice is to
SET_NETDEV_DEV() to your device, which reveals all manner of bus
information by reference.

Storing information in those variables is needless duplication, which is
why they have been relegated only to ancient ISA drivers -- or in the
case of ->mem_start, repurposed as a method of passing options.



>>> + dev->features = NETIF_F_SG;
>>> + if (desc->features & LGUEST_NET_F_NOCSUM)
>>> + dev->features |= NETIF_F_NO_CSUM;
>> do not set SG without an accompanying csum bitflag
>
> That seems... odd. My driver can do SG, and may or may not need csums.
> The current Linux code turns SG off if I need csums and that's fine, but
> it hardly seems like my device should be making that decision.

The net stack does not have a safety cushion. Set the wrong flags, and
things can and will go wrong. SG without CSUM support is illogical, and
can and will break things. Remember what SG is for. If you cannot
offload the csum, you have to build the csum yourself, at which point
you might as well copy it too. grep around for skb_copy_and_csum_dev()
to see if that helps you at all.


>>> +static struct lguest_driver lguestnet_drv = {
>>> + .name = "lguestnet",
>>> + .owner = THIS_MODULE,
>>> + .device_type = LGUEST_DEVICE_T_NET,
>>> + .probe = lguestnet_probe,
>>> +};
>> You are distinctly missing module remove support
>
> Yes. It is never built as a module currently (though it should work).

That's my point. It won't work as a module, because it lacks remove
support. It is not unrealistic to think of [un|re|]loading the net
support module in an lguest guest. And, adding module support makes the
programmer more responsible, because they now have to learn to clean up
after themselves. Any driver that cannot clean up after itself is an
incomplete driver in my book.

Jeff


-
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: 2007-05-10 07:37    [W:0.130 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site