RE: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
> > Thanks a bunch for your really thorough review! I'll answer some of > your questions here. Shreyas can respond to your comments about some of > the coding style/comments/etc. in a separate mail. > > The style is less important at this stage, but certainly eases review > to make it more consistent w/ Linux code. The StudlyCaps, extra macros > (screaming caps) and inconistent space/tabs are visual distractions, > that's all. Agreed, but we'll definitely address all the style issues in our subsequent patch posts. Actually Shreyas showed me his raw patch and it had tabs and not spaces, so we're trying to figure out if either Outlook (corporate blessed) or our Exchange server is converting those tabs to spaces or something. > > We do have an internal prototype of a Linux vmxnet3 driver with 4 Tx > queues and 4 Rx queues, using 9 MSI-X vectors, but it needs some work > before calling it production ready. > > I'd expect once you switch to alloc_etherdev_mq(), make napi work per > rx queue, and fix MSI-X allocation (all needed for 4/4), you should > have enough to support the max of 16/8 (IOW, 4/4 still sounds like an > aritificial limitation). Absolutely: 4/4 was simply a prototype to see if it helps with performance any with certain benchmarks. So far it looks like there's a small performance gain with microbenchmarks like netperf, but we're hoping having multiple queues with multiple vectors might have some promise with macro benchmarks like SPECjbb. If it pans out, we'll most likely make it a module_param with some reasonable defaults, possibly just 1/1 by default. > > > How about GRO conversion? > > > > Looks attractive, and we'll work on that in a subsequent patch. > Again, when we first wrote the driver, the NETIF_F_GRO stuff didn't > exist in Linux. > > OK, shouldn't be too much work. > > Another thing I forgot to mention is that net_device now has > net_device_stats in it. So you shouldn't need net_device_stats in > vmxnet3_adapter. Cool. Will do. > > > > +#define UPT1_MAX_TX_QUEUES 64 > > > > +#define UPT1_MAX_RX_QUEUES 64 > > > > > > This is different than the 16/8 described above (and seemingly all > moot > > > since it becomes a single queue device). > > > > Nice catch! Those are not even used and are from the earliest days of > our driver development. We'll nuke those. > > Could you describe the UPT layer a bit? There were a number of > constants that didn't appear to be used. UPT stands for Uniform Pass Thru, a spec/framework VMware developed with its IHV partners to implement the fast path (Tx/Rx) features of vmxnet3 in silicon. Some of these #defines that appear not to be used are based on this initial spec that VMware shared with its IHV partners. We divided the emulated vmxnet3 PCIe device's registers into two sets on two separate BARs: BAR 0 for the UPT registers we asked IHV partners to implement that we emulate in our hypervisor if no physical device compliant with the UPT spec is available to pass thru from a virtual machine, and BAR 1 for registers we always emulate for slow path/control operations like setting the MAC address, or activating/quiescing/resetting the device, etc. > > > > +static void > > > > +vmxnet3_process_events(struct vmxnet3_adapter *adapter) > > > > > > Should be trivial to break out to it's own MSI-X vector, basically > set > > > up to do that already. > > > > Yes, and the device is configurable to use any vector for any > "events", but didn't see any compelling reason to do so. "ECR" events > are extremely rare and we've got a shadow copy of the ECR register that > avoids an expensive round trip to the device, stored in adapter- > >shared->ecr. So we can cheaply handle events on the hot Tx/Rx path > with minimal overhead. But if you really see a compelling reason to > allocate a separate MSI-X vector for events, we can certainly do that. > > Nah, just thinking outloud while trying to understand the driver. I > figured it'd be the + 1 vector (16 + 8 + 1). Great. In that case we'll stay with not allocating a separate vector for events for now. Thanks! - Bhavesh > > thanks, > -chris ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
Hi Arnd,
> On Tuesday 29 September 2009, Chris Wright wrote:
> > > +struct Vmxnet3_MiscConf {
> > > + struct Vmxnet3_DriverInfo driverInfo;
> > > + uint64_t uptFeatures;
> > > + uint64_t ddPA; /* driver data PA */
> > > + uint64_t queueDescPA; /* queue descriptor
> table PA */
> > > + uint32_t ddLen;/* driver data len */
> > > + uint32_t queueDescLen; /* queue desc. table len
> in bytes */
> > > + uint32_t mtu;
> > > + uint16_t maxNumRxSG;
> > > + uint8_t numTxQueues;
> > > + uint8_t numRxQueues;
> > > + uint32_t reserved[4];
> > > +};
> >
> > should this be packed (or others that are shared w/ device)? i
> assume
> > you've already done 32 vs 64 here
>
> I would not mark it packed, because it already is well-defined on all
> systems. You should add __packed only to the fields where you screwed
> up, but not to structures that already work fine.
You're exactly right; I reiterated as much in my response to Chris.
> One thing that should possibly be fixed is the naming of identifiers,
> e.g.
> 's/Vmxnet3_MiscConf/vmxnet3_misc_conf/g', unless these header files are
> shared with the host implementation.
These header files are indeed shared with the host implementation, as you've
guessed. If it's not a big deal, we would like to keep the names the same, just
for our own sanity's sake?
Thanks!
- Bhavesh
>
> Arnd <><
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
Hi Chris,
Thanks a bunch for your really thorough review! I'll answer some of your
questions here. Shreyas can respond to your comments about some of the coding
style/comments/etc. in a separate mail.
> > INTx, MSI, MSI-X (25 vectors) interrupts
> > 16 Rx queues, 8 Tx queues
>
> Driver doesn't appear to actually support more than a single MSI-X
> interrupt.
> What is your plan for doing real multiqueue?
When we first wrote the driver a couple of years ago, Linux lacked proper
multiqueue support, hence we chose to use only a single queue though the
emulated device does support 16 Rx and 8 Tx queues, and 25 MSI-X vectors: 16
for Rx, 8 for Tx and 1 for other asynchronous event notifications, by design.
Actually a driver can repurpose any of the 25 vectors for any notifications;
just explaining the rationale for desiging the device with 25 MSI-X vectors.
We do have an internal prototype of a Linux vmxnet3 driver with 4 Tx queues and
4 Rx queues, using 9 MSI-X vectors, but it needs some work before calling it
production ready.
> How about GRO conversion?
Looks attractive, and we'll work on that in a subsequent patch. Again, when we
first wrote the driver, the NETIF_F_GRO stuff didn't exist in Linux.
> Also, heavy use of BUG_ON() (counted 51 of them), are you sure that
> none
> of them can be triggered by guest or remote (esp. the ones that happen
> in interrupt context)? Some initial thoughts below.
We'll definitely audit all the BUG_ONs again to make sure they can't be
exploited.
> > --- /dev/null
> > +++ b/drivers/net/vmxnet3/upt1_defs.h
> > +#define UPT1_MAX_TX_QUEUES 64
> > +#define UPT1_MAX_RX_QUEUES 64
>
> This is different than the 16/8 described above (and seemingly all moot
> since it becomes a single queue device).
Nice catch! Those are not even used and are from the earliest days of our
driver development. We'll nuke those.
> > +/* interrupt moderation level */
> > +#define UPT1_IML_NONE 0 /* no interrupt moderation */
> > +#define UPT1_IML_HIGHEST 7 /* least intr generated */
> > +#define UPT1_IML_ADAPTIVE 8 /* adpative intr moderation */
>
> enum? also only appears to support adaptive mode?
Yes, the Linux driver currently only asks for adaptive mode, but the device
supports 8 interrupt moderation levels.
> > --- /dev/null
> > +++ b/drivers/net/vmxnet3/vmxnet3_defs.h
> > +struct Vmxnet3_MiscConf {
> > + struct Vmxnet3_DriverInfo driverInfo;
> > + uint64_t uptFeatures;
> > + uint64_t ddPA; /* driver data PA */
> > + uint64_t queueDescPA; /* queue descriptor table
> PA */
> > + uint32_t ddLen;/* driver data len */
> > + uint32_t queueDescLen; /* queue desc. table len
> in bytes */
> > + uint32_t mtu;
> > + uint16_t maxNumRxSG;
> > + uint8_t numTxQueues;
> > + uint8_t numRxQueues;
> > + uint32_t reserved[4];
> > +};
>
> should this be packed (or others that are shared w/ device)? i assume
> you've already done 32 vs 64 here
>
No need for packing since the fields are naturally 64-bit aligned. True for all
structures shared between the driver and device.
> > +#define VMXNET3_MAX_TX_QUEUES 8
> > +#define VMXNET3_MAX_RX_QUEUES 16
>
> different to UPT, I must've missed some layering here
These are the authoritiative #defines. Ignore the UPT ones.
> > --- /dev/null
> > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> > + VMXNET3_WRITE_BAR0_REG(adapter, VMXNET3_REG_IMR + intr_idx *
> 8, 0);
>
> writel(0, adapter->hw_addr0 + VMXNET3_REG_IMR + intr_idx * 8)
> seems just as clear to me.
Fair enough. We were just trying to clearly show which register accesses go to
BAR 0 versus BAR 1.
> only ever num_intrs=1, so there's some plan to bump this up and make
> these wrappers useful?
Yes.
> > +static void
> > +vmxnet3_process_events(struct vmxnet3_adapter *adapter)
>
> Should be trivial to break out to it's own MSI-X vector, basically set
> up to do that already.
Yes, and the device is configurable to use any vector for any "events", but
didn't see any compelling reason to do so. "ECR" events are extremely rare and
we've got a shadow copy of the ECR register that avoids an expensive round trip
to the device, stored in adapter->shared->ecr. So we can cheaply handle events
on the hot Tx/Rx path with minimal overhead. But if you really see a compelling
reason to allocate a separate MSI-X vector for events, we can certainly do that.
>
> Plan to switch to GRO?
Already answered.
Thanks
- Bhavesh
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
On Tuesday 29 September 2009, David Miller wrote: > > > > These header files are indeed shared with the host implementation, > > as you've guessed. If it's not a big deal, we would like to keep > > the names the same, just for our own sanity's sake? > > No. This isn't your source tree, it's everyone's. So you should > adhere to basic naming conventions and coding standards of the > tree regardless of what you happen to use or need to use internally. Well, there is nothing wrong with making the identifiers the same everywhere, as long as they all follow the Linux coding style ;-). I heard that a number of cross-OS device drivers do that nowadays. Arnd <>< ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
* Bhavesh Davda (bhav...@vmware.com) wrote: > > > Thanks a bunch for your really thorough review! I'll answer some of > > your questions here. Shreyas can respond to your comments about some of > > the coding style/comments/etc. in a separate mail. > > > > The style is less important at this stage, but certainly eases review > > to make it more consistent w/ Linux code. The StudlyCaps, extra macros > > (screaming caps) and inconistent space/tabs are visual distractions, > > that's all. > > Agreed, but we'll definitely address all the style issues in our subsequent > patch posts. Actually Shreyas showed me his raw patch and it had tabs and not > spaces, so we're trying to figure out if either Outlook (corporate blessed) > or our Exchange server is converting those tabs to spaces or something. Ah, that's always fun. You can check by mailing to yourself and looking at the outcome. > > > We do have an internal prototype of a Linux vmxnet3 driver with 4 Tx > > queues and 4 Rx queues, using 9 MSI-X vectors, but it needs some work > > before calling it production ready. > > > > I'd expect once you switch to alloc_etherdev_mq(), make napi work per > > rx queue, and fix MSI-X allocation (all needed for 4/4), you should > > have enough to support the max of 16/8 (IOW, 4/4 still sounds like an > > aritificial limitation). > > Absolutely: 4/4 was simply a prototype to see if it helps with performance > any with certain benchmarks. So far it looks like there's a small performance > gain with microbenchmarks like netperf, but we're hoping having multiple > queues with multiple vectors might have some promise with macro benchmarks > like SPECjbb. If it pans out, we'll most likely make it a module_param with > some reasonable defaults, possibly just 1/1 by default. Most physical devices that do MSI-X will do queue per cpu (or some grouping if large number of cpus compared to queues). Probably reasonable default here too. > > > > How about GRO conversion? > > > > > > Looks attractive, and we'll work on that in a subsequent patch. > > Again, when we first wrote the driver, the NETIF_F_GRO stuff didn't > > exist in Linux. > > > > OK, shouldn't be too much work. > > > > Another thing I forgot to mention is that net_device now has > > net_device_stats in it. So you shouldn't need net_device_stats in > > vmxnet3_adapter. > > Cool. Will do. > > > > > > +#define UPT1_MAX_TX_QUEUES 64 > > > > > +#define UPT1_MAX_RX_QUEUES 64 > > > > > > > > This is different than the 16/8 described above (and seemingly all > > moot > > > > since it becomes a single queue device). > > > > > > Nice catch! Those are not even used and are from the earliest days of > > our driver development. We'll nuke those. > > > > Could you describe the UPT layer a bit? There were a number of > > constants that didn't appear to be used. > > UPT stands for Uniform Pass Thru, a spec/framework VMware developed with its > IHV partners to implement the fast path (Tx/Rx) features of vmxnet3 in > silicon. Some of these #defines that appear not to be used are based on this > initial spec that VMware shared with its IHV partners. > > We divided the emulated vmxnet3 PCIe device's registers into two sets on two > separate BARs: BAR 0 for the UPT registers we asked IHV partners to implement > that we emulate in our hypervisor if no physical device compliant with the > UPT spec is available to pass thru from a virtual machine, and BAR 1 for > registers we always emulate for slow path/control operations like setting the > MAC address, or activating/quiescing/resetting the device, etc. Interesting. Sounds like part of NetQueue and also something that virtio has looked into to support, e.g. VMDq. Do you have a more complete spec? thanks, -chris ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
Chris,
Thanks for the review.
Bhavesh responded to some queries. I will attempt the answer the rest.
I am working on rebasing the code to v2.6.32-rc1. Will send out a patch
with the changes you suggested after that.
->Shreyas
> -Original Message-
> From: Chris Wright [mailto:chr...@sous-sol.org]
> Sent: Tuesday, September 29, 2009 1:54 AM
> To: Shreyas Bhatewara
> Cc: linux-ker...@vger.kernel.org; net...@vger.kernel.org; Stephen
> Hemminger; David S. Miller; Jeff Garzik; Anthony Liguori; Chris Wright;
> Greg Kroah-Hartman; Andrew Morton; virtualization; pv-
> driv...@vmware.com
> Subject: Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC
> driver: vmxnet3
>
>
> > Wake-on-LAN, PCI Power Management D0-D3 states
> > PXE-ROM for boot support
> >
>
> Whole thing appears to be space indented, and is fairly noisy w/
> printk.
The code written is tab indented. Is there a specific line / function
which you find space indented ? If not, may be my email client did not
preserve the tabs while sending. I will take care while posting next patch.
Some of the printks are debug only. Others have been marked as INFO or ERR
appropriately. I will remove a few.
> Also, heavy use of BUG_ON() (counted 51 of them), are you sure that
> none
> of them can be triggered by guest or remote (esp. the ones that happen
> in interrupt context)? Some initial thoughts below.
>
Like you said below, I will remove the user input dependent ones.
> > + * the file called "COPYING".
> > + *
> > + * Maintained by: Shreyas Bhatewara
> > + *
> > + */
> > +
> > +/* upt1_defs.h
> > + *
> > + * Definitions for Uniform Pass Through.
> > + */
>
> Most of the source files have this format (some include -- after file
> name). Could just keep it all w/in the same comment block. Since you
> went to the trouble of saying what the file does, something a tad more
> descriptive would be welcome.
>
Yes, I will merge the two blocks and elaborate on the description.
> > +
> > +#ifndef _UPT1_DEFS_H
> > +#define _UPT1_DEFS_H
> > +
> > +#define UPT1_MAX_TX_QUEUES 64
> > +#define UPT1_MAX_RX_QUEUES 64
>
> This is different than the 16/8 described above (and seemingly all moot
> since it becomes a single queue device).
>
> > +
> > +/* interrupt moderation level */
> > +#define UPT1_IML_NONE 0 /* no interrupt moderation */
> > +#define UPT1_IML_HIGHEST 7 /* least intr generated */
> > +#define UPT1_IML_ADAPTIVE 8 /* adpative intr moderation */
>
> enum? also only appears to support adaptive mode?
> > +
> > +/*
> > + * QueueDescPA must be 128 bytes aligned. It points to an array of
> > + * Vmxnet3_TxQueueDesc followed by an array of Vmxnet3_RxQueueDesc.
> > + * The number of Vmxnet3_TxQueueDesc/Vmxnet3_RxQueueDesc are
> specified by
> > + * Vmxnet3_MiscConf.numTxQueues/numRxQueues, respectively.
> > + */
> > +#define VMXNET3_QUEUE_DESC_ALIGN 128
>
> Lot of inconsistent spacing between types and names in the structure
> def'ns
Okay, I will try to make it uniform.
>
> > +struct Vmxnet3_MiscConf {
> > + struct Vmxnet3_DriverInfo driverInfo;
> > + uint64_t uptFeatures;
> > + uint64_t ddPA; /* driver data PA */
> > + uint64_t queueDescPA; /* queue descriptor table
> PA */
> > + uint32_t ddLen;/* driver data len */
> > + uint32_t queueDescLen; /* queue desc. table len
> in bytes */
> > + uint32_t mtu;
> > + uint16_t maxNumRxSG;
> > + uint8_t numTxQueues;
> > + uint8_t numRxQueues;
> > + uint32_t reserved[4];
> > +};
>
> should this be packed (or others that are shared w/ device)? i assume
> you've already done 32 vs 64 here
>
> > +struct Vmxnet3_TxQueueConf {
> > + uint64_ttxRingBasePA;
> > + uint64_tdataRingBasePA;
> > + uint64_tcompRingBasePA;
> > + uint64_tddPA; /* driver data */
> > + uint64_treserved;
> > + uint32_ttxRingSize; /* # of tx desc */
> > + uint32_tdataRingSize; /* # of data desc */
> > + uint32_tcompRingSize; /* # of comp desc */
> > + uint32_tddLen;/* size of driver data */
> > + uint8_t intrIdx;
> > + uint8_t _pad[7];
> > +};
> > +
> > +
> > +struct Vmxnet3_RxQueueConf {
> > + uint64_trxRingBasePA[2];
> > + uint
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
* Bhavesh Davda (bhav...@vmware.com) wrote:
> Hi Chris,
>
> Thanks a bunch for your really thorough review! I'll answer some of your
> questions here. Shreyas can respond to your comments about some of the coding
> style/comments/etc. in a separate mail.
The style is less important at this stage, but certainly eases review
to make it more consistent w/ Linux code. The StudlyCaps, extra macros
(screaming caps) and inconistent space/tabs are visual distractions,
that's all.
> > > INTx, MSI, MSI-X (25 vectors) interrupts
> > > 16 Rx queues, 8 Tx queues
> >
> > Driver doesn't appear to actually support more than a single MSI-X
> > interrupt.
> > What is your plan for doing real multiqueue?
>
> When we first wrote the driver a couple of years ago, Linux lacked proper
> multiqueue support, hence we chose to use only a single queue though the
> emulated device does support 16 Rx and 8 Tx queues, and 25 MSI-X vectors: 16
> for Rx, 8 for Tx and 1 for other asynchronous event notifications, by design.
> Actually a driver can repurpose any of the 25 vectors for any notifications;
> just explaining the rationale for desiging the device with 25 MSI-X vectors.
I see, thanks.
> We do have an internal prototype of a Linux vmxnet3 driver with 4 Tx queues
> and 4 Rx queues, using 9 MSI-X vectors, but it needs some work before calling
> it production ready.
I'd expect once you switch to alloc_etherdev_mq(), make napi work per
rx queue, and fix MSI-X allocation (all needed for 4/4), you should
have enough to support the max of 16/8 (IOW, 4/4 still sounds like an
aritificial limitation).
> > How about GRO conversion?
>
> Looks attractive, and we'll work on that in a subsequent patch. Again, when
> we first wrote the driver, the NETIF_F_GRO stuff didn't exist in Linux.
OK, shouldn't be too much work.
Another thing I forgot to mention is that net_device now has
net_device_stats in it. So you shouldn't need net_device_stats in
vmxnet3_adapter.
> > Also, heavy use of BUG_ON() (counted 51 of them), are you sure that
> > none
> > of them can be triggered by guest or remote (esp. the ones that happen
> > in interrupt context)? Some initial thoughts below.
>
> We'll definitely audit all the BUG_ONs again to make sure they can't be
> exploited.
>
> > > --- /dev/null
> > > +++ b/drivers/net/vmxnet3/upt1_defs.h
> > > +#define UPT1_MAX_TX_QUEUES 64
> > > +#define UPT1_MAX_RX_QUEUES 64
> >
> > This is different than the 16/8 described above (and seemingly all moot
> > since it becomes a single queue device).
>
> Nice catch! Those are not even used and are from the earliest days of our
> driver development. We'll nuke those.
Could you describe the UPT layer a bit? There were a number of
constants that didn't appear to be used.
> > > +/* interrupt moderation level */
> > > +#define UPT1_IML_NONE 0 /* no interrupt moderation */
> > > +#define UPT1_IML_HIGHEST 7 /* least intr generated */
> > > +#define UPT1_IML_ADAPTIVE 8 /* adpative intr moderation */
> >
> > enum? also only appears to support adaptive mode?
>
> Yes, the Linux driver currently only asks for adaptive mode, but the device
> supports 8 interrupt moderation levels.
>
> > > --- /dev/null
> > > +++ b/drivers/net/vmxnet3/vmxnet3_defs.h
> > > +struct Vmxnet3_MiscConf {
> > > + struct Vmxnet3_DriverInfo driverInfo;
> > > + uint64_t uptFeatures;
> > > + uint64_t ddPA; /* driver data PA */
> > > + uint64_t queueDescPA; /* queue descriptor table
> > PA */
> > > + uint32_t ddLen;/* driver data len */
> > > + uint32_t queueDescLen; /* queue desc. table len
> > in bytes */
> > > + uint32_t mtu;
> > > + uint16_t maxNumRxSG;
> > > + uint8_t numTxQueues;
> > > + uint8_t numRxQueues;
> > > + uint32_t reserved[4];
> > > +};
> >
> > should this be packed (or others that are shared w/ device)? i assume
> > you've already done 32 vs 64 here
> >
>
> No need for packing since the fields are naturally 64-bit aligned. True for
> all structures shared between the driver and device.
I had quickly looked and thought I saw a hole that would lead to
inconsistent layout for 32-bit vs 64-bit. I figured I'd be wrong
there ;-)
> > > +#define VMXNET3_MAX_TX_QUEUES 8
> > > +#define VMXNET3_MAX_RX_QUEUES 16
> >
> > different to UPT, I must've missed some layering here
>
> These are the authoritiative #defines. Ignore the UPT ones.
>
> > > --- /dev/null
> > > +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> > > + VMXNET3_WRITE_BAR0_REG(adapter, VMXNET3_REG_IMR + intr_idx *
> > 8, 0);
> >
> > writel(0, adapter->hw_addr0 + VMXNET3_REG_IMR + intr_idx * 8)
> > seems just as clear to me.
>
> Fair enough. We were just trying to clearly show which register accesses go
> to BAR 0 versus BAR 1.
>
> > only ever num_intrs=1, so there's some p
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
From: Bhavesh Davda Date: Tue, 29 Sep 2009 12:52:05 -0700 >> One thing that should possibly be fixed is the naming of identifiers, >> e.g. >> 's/Vmxnet3_MiscConf/vmxnet3_misc_conf/g', unless these header files are >> shared with the host implementation. > > These header files are indeed shared with the host implementation, as you've > guessed. If it's not a big deal, we would like to keep the names the same, > just for our own sanity's sake? No. This isn't your source tree, it's everyone's. So you should adhere to basic naming conventions and coding standards of the tree regardless of what you happen to use or need to use internally. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
> -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Monday, September 28, 2009 5:08 PM > To: Shreyas Bhatewara > Cc: linux-ker...@vger.kernel.org; net...@vger.kernel.org; > shemmin...@linux-foundation.org; jgar...@pobox.com; > anth...@codemonkey.ws; chr...@sous-sol.org; g...@kroah.com; a...@linux- > foundation.org; virtualization@lists.linux-foundation.org; pv- > driv...@vmware.com > Subject: Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC > driver: vmxnet3 > > From: Shreyas Bhatewara > Date: Mon, 28 Sep 2009 16:56:45 -0700 > > > + uint32_t rxdIdx:12;/* Index of the RxDesc */ > > Don't use uint32_t et al. sized types, use "u32" and friends > throughout. Sure, I will fix that. ->Shreyas ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
On Tuesday 29 September 2009, Chris Wright wrote:
> > +struct Vmxnet3_MiscConf {
> > + struct Vmxnet3_DriverInfo driverInfo;
> > + uint64_t uptFeatures;
> > + uint64_t ddPA; /* driver data PA */
> > + uint64_t queueDescPA; /* queue descriptor table PA */
> > + uint32_t ddLen;/* driver data len */
> > + uint32_t queueDescLen; /* queue desc. table len in
> > bytes */
> > + uint32_t mtu;
> > + uint16_t maxNumRxSG;
> > + uint8_t numTxQueues;
> > + uint8_t numRxQueues;
> > + uint32_t reserved[4];
> > +};
>
> should this be packed (or others that are shared w/ device)? i assume
> you've already done 32 vs 64 here
I would not mark it packed, because it already is well-defined on all
systems. You should add __packed only to the fields where you screwed
up, but not to structures that already work fine.
One thing that should possibly be fixed is the naming of identifiers, e.g.
's/Vmxnet3_MiscConf/vmxnet3_misc_conf/g', unless these header files are
shared with the host implementation.
Arnd <><
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
* Shreyas Bhatewara (sbhatew...@vmware.com) wrote:
> Some of the features of vmxnet3 are :
> PCIe 2.0 compliant PCI device: Vendor ID 0x15ad, Device ID 0x07b0
> INTx, MSI, MSI-X (25 vectors) interrupts
> 16 Rx queues, 8 Tx queues
Driver doesn't appear to actually support more than a single MSI-X interrupt.
What is your plan for doing real multiqueue?
> Offloads: TCP/UDP checksum, TSO over IPv4/IPv6,
> 802.1q VLAN tag insertion, filtering, stripping
> Multicast filtering, Jumbo Frames
How about GRO conversion?
> Wake-on-LAN, PCI Power Management D0-D3 states
> PXE-ROM for boot support
>
Whole thing appears to be space indented, and is fairly noisy w/ printk.
Also, heavy use of BUG_ON() (counted 51 of them), are you sure that none
of them can be triggered by guest or remote (esp. the ones that happen
in interrupt context)? Some initial thoughts below.
> diff --git a/drivers/net/vmxnet3/upt1_defs.h b/drivers/net/vmxnet3/upt1_defs.h
> new file mode 100644
> index 000..b50f91b
> --- /dev/null
> +++ b/drivers/net/vmxnet3/upt1_defs.h
> @@ -0,0 +1,104 @@
> +/*
> + * Linux driver for VMware's vmxnet3 ethernet NIC.
> + *
> + * Copyright (C) 2008-2009, VMware, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; version 2 of the License and no later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Maintained by: Shreyas Bhatewara
> + *
> + */
> +
> +/* upt1_defs.h
> + *
> + * Definitions for Uniform Pass Through.
> + */
Most of the source files have this format (some include -- after file
name). Could just keep it all w/in the same comment block. Since you
went to the trouble of saying what the file does, something a tad more
descriptive would be welcome.
> +
> +#ifndef _UPT1_DEFS_H
> +#define _UPT1_DEFS_H
> +
> +#define UPT1_MAX_TX_QUEUES 64
> +#define UPT1_MAX_RX_QUEUES 64
This is different than the 16/8 described above (and seemingly all moot
since it becomes a single queue device).
> +
> +/* interrupt moderation level */
> +#define UPT1_IML_NONE 0 /* no interrupt moderation */
> +#define UPT1_IML_HIGHEST 7 /* least intr generated */
> +#define UPT1_IML_ADAPTIVE 8 /* adpative intr moderation */
enum? also only appears to support adaptive mode?
> +/* values for UPT1_RSSConf.hashFunc */
> +enum {
> + UPT1_RSS_HASH_TYPE_NONE = 0x0,
> + UPT1_RSS_HASH_TYPE_IPV4 = 0x01,
> + UPT1_RSS_HASH_TYPE_TCP_IPV4 = 0x02,
> + UPT1_RSS_HASH_TYPE_IPV6 = 0x04,
> + UPT1_RSS_HASH_TYPE_TCP_IPV6 = 0x08,
> +};
> +
> +enum {
> + UPT1_RSS_HASH_FUNC_NONE = 0x0,
> + UPT1_RSS_HASH_FUNC_TOEPLITZ = 0x01,
> +};
> +
> +#define UPT1_RSS_MAX_KEY_SIZE40
> +#define UPT1_RSS_MAX_IND_TABLE_SIZE 128
> +
> +struct UPT1_RSSConf {
> + uint16_t hashType;
> + uint16_t hashFunc;
> + uint16_t hashKeySize;
> + uint16_t indTableSize;
> + uint8_thashKey[UPT1_RSS_MAX_KEY_SIZE];
> + uint8_tindTable[UPT1_RSS_MAX_IND_TABLE_SIZE];
> +};
> +
> +/* features */
> +enum {
> + UPT1_F_RXCSUM = 0x0001, /* rx csum verification */
> + UPT1_F_RSS = 0x0002,
> + UPT1_F_RXVLAN = 0x0004, /* VLAN tag stripping */
> + UPT1_F_LRO = 0x0008,
> +};
> +#endif
> diff --git a/drivers/net/vmxnet3/vmxnet3_defs.h
> b/drivers/net/vmxnet3/vmxnet3_defs.h
> new file mode 100644
> index 000..a33a90b
> --- /dev/null
> +++ b/drivers/net/vmxnet3/vmxnet3_defs.h
> @@ -0,0 +1,534 @@
> +/*
> + * Linux driver for VMware's vmxnet3 ethernet NIC.
> + *
> + * Copyright (C) 2008-2009, VMware, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; version 2 of the License and no later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
From: Alok Kataria Date: Mon, 28 Sep 2009 17:51:39 -0700 > As a side note, were there any changes in the networking API's, that we > should look out for in the merge cycle ? > If not I think the rebase should be pretty trivial. Just off the top of my head, the return type of the driver transmit function was changed to netdev_tx_t, for one thing. But there were likely numerous others. You'll have to check. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
On Mon, 2009-09-28 at 17:22 -0700, Greg KH wrote: > On Mon, Sep 28, 2009 at 04:56:45PM -0700, Shreyas Bhatewara wrote: > > The patch applies to 2.6.31-rc9. > > 2.6.32-rc1 is out, you should rebase to it as a few tens of thousands of > changes have already happened since 2.6.31-rc9 :) > Yep, we should rebase this, we will do that while incorporating the comments that we got from David, and any others that you think we should be making. As a side note, were there any changes in the networking API's, that we should look out for in the merge cycle ? If not I think the rebase should be pretty trivial. Thanks, Alok > thanks, > > greg k-h > ___ > Pv-drivers mailing list > pv-driv...@vmware.com > http://mailman2.vmware.com/mailman/listinfo/pv-drivers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
Hi Greg, On Mon, 2009-09-28 at 17:20 -0700, Greg KH wrote: > On Mon, Sep 28, 2009 at 04:56:45PM -0700, Shreyas Bhatewara wrote: > > Ethernet NIC driver for VMware's vmxnet3 > > > > From: Shreyas Bhatewara > > > > This patch adds driver support for VMware's virtual Ethernet NIC : vmxnet3 > > Guests running on VMware hypervisors supporting vmxnet3 device will thus > > have access to improved network functionalities and performance. > > > > Signed-off-by: Shreyas Bhatewara > > I thought this was going to be submitted for the drivers/staging/ tree. > What happened? We managed to do most of the cleanup's inhouse over the weekend and think this shouldn't need any major cleanup now. That's why thought better to submit directly. Thanks, Alok > > thanks, > > greg k-h > ___ > Pv-drivers mailing list > pv-driv...@vmware.com > http://mailman2.vmware.com/mailman/listinfo/pv-drivers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
On Mon, Sep 28, 2009 at 04:56:45PM -0700, Shreyas Bhatewara wrote: > > Please consider this for inclusion in the linux net tree. I will be > glad to receive your review comments and answer queries in order to be > accepted in mainline in 2.6.32 release cycle. It's usually a bit late given that the big merge window for 2.6.32 is now closed. > The patch applies to 2.6.31-rc9. 2.6.32-rc1 is out, you should rebase to it as a few tens of thousands of changes have already happened since 2.6.31-rc9 :) thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
On Mon, Sep 28, 2009 at 04:56:45PM -0700, Shreyas Bhatewara wrote: > Ethernet NIC driver for VMware's vmxnet3 > > From: Shreyas Bhatewara > > This patch adds driver support for VMware's virtual Ethernet NIC : vmxnet3 > Guests running on VMware hypervisors supporting vmxnet3 device will thus > have access to improved network functionalities and performance. > > Signed-off-by: Shreyas Bhatewara I thought this was going to be submitted for the drivers/staging/ tree. What happened? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [PATCH 2.6.31-rc9] net: VMware virtual Ethernet NIC driver: vmxnet3
From: Shreyas Bhatewara Date: Mon, 28 Sep 2009 16:56:45 -0700 > + uint32_t rxdIdx:12;/* Index of the RxDesc */ Don't use uint32_t et al. sized types, use "u32" and friends throughout. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
