Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
On Wed, Jul 14, 2010 at 01:42:59PM -0700, Shreyas Bhatewara wrote:
> +/* vmkernel and device backend shared definitions */
> +
> +#define VMXNET3_PLUGIN_NAME_LEN 256
> +#define VMXNET3_PLUGIN_REPOSITORY "/usr/lib/vmware/npa_plugins"
Why would the kernel care about this file path? And since when do we
hard-code file paths in the kernel in the first place (yeah, in some
places we do, but not like this...)
> +#define NPA_MEMIO_REGIONS_u64X6
> +
> +typedef u32 VF_ID;
> +
> +struct Vmxnet3_VFInfo {
> + char pluginName[VMXNET3_PLUGIN_NAME_LEN];
This is never used.
> + u32 deviceInfo[VMXNET3_PLUGIN_INFO_LEN]; /* opaque data returned
> + * by PF driver */
This is happily copied around and zeroed out, but never actually used by
anything.
> + u64 memioAddr;
> + u32 memioLen;
This field is never used.
Why have fields in a structure that are never used?
> +};
<...>
> +/*
> + * Easy shell API calling macros.
> + */
> +#define Shell_AllocSmallBuffer(_state, _handle, _ringOffset) \
> + ((_state)->shellApi.allocSmallBuffer((_handle), (_ringOffset)))
> +#define Shell_AllocLargeBuffer(_state, _handle, _ringOffset) \
> + ((_state)->shellApi.allocLargeBuffer((_handle), (_ringOffset)))
> +#define Shell_FreeBuffer(_state, _handle, _ringOffset)
> \
> + ((_state)->shellApi.freeBuffer((_handle), (_ringOffset)))
> +#define Shell_CompleteSend(_state, _handle, _numPkt) \
> + ((_state)->shellApi.completeSend((_handle), (_numPkt)))
> +#define Shell_IndicateRecv(_state, _handle, _frame) \
> + ((_state)->shellApi.indicateRecv((_handle), (_frame)))
> +#define Shell_Log(_state, _loglevel, _n, _fmt, ...) \
> + do {\
> + if (logEnabled && (_loglevel) <= (u32)logLevel) { \
> + (_state)->shellApi.log((_n) + 1,\
> + "%s: " _fmt,\
> + __func__, \
> +##__VA_ARGS__); \
> + } \
> + } while (0)
This hiding of functions kind of implies that something odd is going on
here, right? At the least, make them inline functions so you get the
proper typechecking warnings/errors in a format that you can understand.
> +/*
> + * Some standard definitions
> + */
> +#ifndef NULL
> +#define NULL (void *)0
> +#endif
What's wrong with the kernel-provided version of this?
> +/*
> + * Utility macro to write a register's value (BAR0)
> + */
> +#define VMXNET3_WRITE_REG(_state, _offset, _value) \
> + (*(u32 *)((u8 *)(_state)->memioAddr + (_offset)) = \
> + (_value))
This will never work, sorry. Please use the proper functions for doing
this type of access. I'm amazed that anyone even thought this would
succeed...
> +/*
> + * Utility macro to align a virtual address
> + */
> +#define ALIGN_VA(_ptr, _align) ((void *)(((uintptr_t)(_ptr) + ((_align) -
> 1)) &\
> + ~((_align) - 1)))
What's wrong with the kernel provided function for this?
Anyway, just randomly poking at the code like this turns up these types
of trivial issues, has this code ever been run?
wierd,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
On Wed, Jul 14, 2010 at 10:18:22AM -0700, Pankaj Thakkar wrote: > The plugin is guest agnostic and hence we did not want to rely on any > kernel provided functions. The plugin uses only the interface provided > by the shell. Really? vmxnet3_plugin.c is no supposed to use any kernel-provided functions at all? Then why have it in the kernel at all? Seriously, why? > The assumption is that since the plugin is really simple and straight > forward (all the control/init complexity lies in the PF driver in the > hypervisor) we should be able to get by for most of the things and for > things like memcpy/memset the plugin can write simple functions like > this. If it's so simple, then why does it need to be separate? Why not just put it in your driver as-is to handle the ring-buffer logic (as that's all it looks to be doing), and then you don't need any plugin code at all? It looks like you are linking this file into your "main" driver module, so I fail to see any type of separation at all happening with this patch. Or am I totally missing something here? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
On 07/14/2010 10:54 AM, David Miller wrote: > And doing what you're doing is foolish on so many levels. One more > duplication of code, one more place for unnecessary bugs to live, one > more place that might need optimizations and thus require duplication > of even more work people have done over the years. > Not to mention calling a function "MoveMemory" when it doesn't do a memmove is just cruel. J ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Pankaj Thakkar Date: Wed, 14 Jul 2010 10:18:22 -0700 > The plugin is guest agnostic and hence we did not want to rely on > any kernel provided functions. While I disagree entirely with this kind of approach, even that doesn't justify what you're doing here. memcpy() and memset() are on a much more fundamental ground than "kernel provided functions". They had better be available no matter where you build this thing. And doing what you're doing is foolish on so many levels. One more duplication of code, one more place for unnecessary bugs to live, one more place that might need optimizations and thus require duplication of even more work people have done over the years. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
RE: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
The plugin is guest agnostic and hence we did not want to rely on any kernel provided functions. The plugin uses only the interface provided by the shell. The assumption is that since the plugin is really simple and straight forward (all the control/init complexity lies in the PF driver in the hypervisor) we should be able to get by for most of the things and for things like memcpy/memset the plugin can write simple functions like this. -p From: Greg KH [g...@kroah.com] Sent: Wednesday, July 14, 2010 2:49 AM To: Shreyas Bhatewara Cc: Christoph Hellwig; Stephen Hemminger; Pankaj Thakkar; pv-driv...@vmware.com; net...@vger.kernel.org; linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org Subject: Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3 Is there some reason that our in-kernel functions that do this type of logic are not working for you to require you to reimplement this? thanks, greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
On Wed, 14 Jul 2010, Greg KH wrote:
> On Mon, Jul 12, 2010 at 08:06:28PM -0700, Shreyas Bhatewara wrote:
> > drivers/net/vmxnet3/vmxnet3_drv.c | 1845
> > +++--
>
> Your patch is line-wrapped and can not be applied :(
>
> Care to fix your email client?
>
> One thing just jumped out at me when glancing at this:
>
> > +static INLINE void
> > +MoveMemory(void *dst,
> > + void *src,
> > + size_t length)
> > +{
> > + size_t i;
> > + for (i = 0; i < length; ++i)
> > + ((u8 *)dst)[i] = ((u8 *)src)[i];
> > +}
> > +
> > +static INLINE void
> > +ZeroMemory(void *memory,
> > + size_t length)
> > +{
> > + size_t i;
> > + for (i = 0; i < length; ++i)
> > + ((u8 *)memory)[i] = 0;
> > +}
>
> Is there some reason that our in-kernel functions that do this type of
> logic are not working for you to require you to reimplement this?
>
> thanks,
>
> greg k-h
>
Greg,
Thanks for pointing out. I will fix both these issues and repost the patch.
->Shreyas
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: Virtualization at Plumbers 2010 - Time to submit your proposals!
Hi, I would like to remind everybody that the deadline for submitting a proposal to Linux Plumbers 2010 is nearing, so this is the time to get your proposals in. The deadline is July 19th! There has been some confusion about the submission process and I have only just found out myself what is the correct way to do it. You need to submit your proposal via the following link: http://www.linuxplumbersconf.org/2010/ocw/events/LPC2010MC/proposals If you have already submitted a Virtualization related proposal please check that it went in to the mini conference submission. If it didn't please re-submit it. I apologize for this confusion! Hope to see plenty of proposals showing up before the deadling on the 19th! Best regards, Jes On 06/18/10 14:00, Jes Sorensen wrote: > Hi, > > I would like to remind people about the Virtualization track at Linux > Plumbers Conference 2010, held in Cambridge, MA, November 3-5, 2010. > > Please note the deadline for submissions is July 19, 2010. > > LPC is particular well suited for technical presentations, work in > progress and subjects that needs discussion and collaboration between > communities (kernel, desktop/gfx, virtualization, etc.), so if you have > a contentious issue you would like to bring to a wider audience, this is > the ideal place to do it! > > Note that this track is focusing on general Linux Virtualization, it > is not hypervisor specific. Submissions related to Xen, KVM, VMware, > containers, etc. are encouraged. Subjects could include: > > - Linux Kernel Virtualization enhancements > - QEMU > - IO performance work > - Device management, hotplug > - NUMA awareness > - Live migration > - Support for new hardware features, and/or provide guest access to >these features. > - Device emulation > - Para-virtual enhancements: special filesystems, PMU, Windows >drivers, etc. > - Debugging and analysis tools > - Containers > - QMP and Spice > - Virtualization management, user interfaces, and desktop integration >(GNOME, KDE, etc) > > If you have a subject you would like to present, please submit it as > soon as possible, and no later than July 19th. Please see the full Call > For Papers at http://www.linuxplumbersconf.org/2010/ for how to submit. > > You may also want to list your ideas at the LPC Virtualization session > wiki page at http://wiki.linuxplumbersconf.org/2010:virtualization > > Hope to see you in Cambridge in November! > > Jes ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/virtualization
Re: [Pv-drivers] RFC: Network Plugin Architecture (NPA) for vmxnet3
On Mon, Jul 12, 2010 at 08:06:28PM -0700, Shreyas Bhatewara wrote:
> drivers/net/vmxnet3/vmxnet3_drv.c | 1845
> +++--
Your patch is line-wrapped and can not be applied :(
Care to fix your email client?
One thing just jumped out at me when glancing at this:
> +static INLINE void
> +MoveMemory(void *dst,
> + void *src,
> + size_t length)
> +{
> + size_t i;
> + for (i = 0; i < length; ++i)
> + ((u8 *)dst)[i] = ((u8 *)src)[i];
> +}
> +
> +static INLINE void
> +ZeroMemory(void *memory,
> + size_t length)
> +{
> + size_t i;
> + for (i = 0; i < length; ++i)
> + ((u8 *)memory)[i] = 0;
> +}
Is there some reason that our in-kernel functions that do this type of
logic are not working for you to require you to reimplement this?
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
