From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tu3p0-0001nW-Cv for qemu-devel@nongnu.org; Sat, 12 Jan 2013 11:20:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tu3ov-00063Z-3h for qemu-devel@nongnu.org; Sat, 12 Jan 2013 11:20:38 -0500 Received: from mail-ia0-f178.google.com ([209.85.210.178]:48376) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tu3ou-00062z-TA for qemu-devel@nongnu.org; Sat, 12 Jan 2013 11:20:33 -0500 Received: by mail-ia0-f178.google.com with SMTP id k25so2418916iah.37 for ; Sat, 12 Jan 2013 08:20:32 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20130107141411.GB18749@stefanha-thinkpad.redhat.com> References: <1354878909-21369-1-git-send-email-dmitry@daynix.com> <20130107141411.GB18749@stefanha-thinkpad.redhat.com> Date: Sat, 12 Jan 2013 18:20:32 +0200 Message-ID: From: Dmitry Fleytman Content-Type: multipart/alternative; boundary=f46d042fd84687d0cb04d319caff Subject: Re: [Qemu-devel] [PATCH V8 0/5] VMXNET3 paravirtual NIC device implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Yan Vugenfirer , Gerhard Wiesinger , qemu-devel , Anthony Liguori , Paolo Bonzini --f46d042fd84687d0cb04d319caff Content-Type: text/plain; charset=ISO-8859-1 On Mon, Jan 7, 2013 at 4:14 PM, Stefan Hajnoczi wrote: > On Fri, Dec 07, 2012 at 01:15:04PM +0200, Dmitry Fleytman wrote: > > > +struct eth_header { > > > + uint8_t h_dest[ETH_ALEN]; /* destination eth addr */ > > > + uint8_t h_source[ETH_ALEN]; /* source ether addr */ > > > + uint16_t h_proto; /* packet type ID field */ > > > +}; > > > > Looks like it's copy-pasted stuff from /usr/include/linux/if_*.h, > > /usr/include/netinet/*.h, and friends. If the system-wide headers are > > included names will collide for some of the macros at least. > > > > Did you check if the slirp/ definitions can be reused? > > > > [DF] Yes, you are right. This is copy-pasted from different places. > > [DF] Slips definishing do not fully cover our needs. > > > > > > I'd rather we import network header definitions once in a generic > > place into the source tree. That way vmxnet and other components > > don't need to redefine these structs. > > > > [DF] Exaclty! Our intention is to create generic header with network > definitions and make everyone use it. > > [DF] We can move our header to some shared place if you want, however > I'd do it in parallel with cleanup > > [DF] of similar definitions in existing code and this is a big change > that os out of scope of these patches. > > Please put it in a generic place in include/qemu/.... Please > double-check copyright headers you need when copy-pasting code from > various system headers. > > Done. > > > + > > > > *===========================================================================*/ > > > > Is this huge comment box a sign that the code should be split into a > > foo_tx.c and an foo_rx.c file? > > > > [DF] As for me this file is not that big to be splitted (<800 lines), > however I'll do this if you insist :) > > It came to mind because I find comment boxes ugly. They tend to be > pointless "*********** PUBLIC METHODS ***********" or they show that the > code should really be split. Just a pet peeve but please do split it if > possible. > > Did it. > > > +static inline void vmxnet3_flush_shmem_changes(void) > > > +{ > > > + /* > > > + * Flush shared memory changes > > > + * Needed before sending interrupt to guest to ensure > > > + * it gets consistent memory state > > > + */ > > > + smp_wmb(); > > > +} > > > > It's useful to document why a memory barrier is being used in each > > instance. Therefore hiding smp_wmb() inside a wrapper function isn't > > great. > > > > [DF] Fixed. > > > > Also, it's suspicious that smb_wmb() is used but no other barriers are > > used. What about a read memory barrier when accessing shared memory > > written by the guest? > > > > [DF] VMWARE interface build a in safe way - you always read out data > buffer (packet descriptor) with memcopy, > > [DF] and then check its last byte to see whether it was fully filled by > driver. > > [DF] In this case device doesn't need read barriers. Drivers need write > barriers of course to secure > > [DF] proper order of writes. > > That's not portable, you're making assumptions about memory ordering > which varies from architecture to architecture. Some of the barriers > compile out on x86 because the memory model is strong. > > QEMU is portable across host architectures so you need to use the read > memory barrier. > > Stefan > Oh, I see, thanks for explanation. Fixed. -- Dmitry Fleytman Technology Expert and Consultant, Daynix Computing Ltd. Cell: +972-54-2819481 Skype: dmitry.fleytman --f46d042fd84687d0cb04d319caff Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



On Mon, Jan 7, 2013 at 4:14 PM, Stefan Hajnoczi &= lt;stefanha@gmail.c= om> wrote:
On Fri, Dec 07, 2012 at 01= :15:04PM +0200, Dmitry Fleytman wrote:
> > +struct eth_header {
> > + =A0 =A0uint8_t =A0h_dest[ETH_ALEN]; =A0 /* destination eth addr= */
> > + =A0 =A0uint8_t =A0h_source[ETH_ALEN]; /* source ether addr =A0 = =A0*/
> > + =A0 =A0uint16_t h_proto; =A0 =A0 =A0 =A0 =A0 =A0/* packet type = ID field */
> > +};
>
> Looks like it's copy-pasted stuff from /usr/include/linux/if_*.h,<= br> > /usr/include/netinet/*.h, and friends. =A0If the system-wide headers a= re
> included names will collide for some of the macros at least.
>
> Did you check if the slirp/ definitions can be reused?
>
> [DF] Yes, you are right. This is copy-pasted from different places. > [DF] Slips definishing do not fully cover our needs.
>
>
> I'd rather we import network header definitions once in a generic<= br> > place into the source tree. =A0That way vmxnet and other components > don't need to redefine these structs.
>
> [DF] Exaclty! Our intention is to create generic header with network d= efinitions and make everyone use it.
> [DF] We can move our header to some shared place if you want, however = I'd do it in parallel with cleanup
> [DF] of similar definitions in existing code and this is a big change = that os out of scope of these patches.

Please put it in a generic place in include/qemu/.... =A0Please
double-check copyright headers you need when copy-pasting code from
various system headers.


Done.


=A0
> > +
> > *=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D*/
>
> Is this huge comment box a sign that the code should be split into a > foo_tx.c and an foo_rx.c file?
>
> [DF] As for me this file is not that big to be splitted (<800 lines= ), however I'll do this if you insist :)

It came to mind because I find comment boxes ugly. =A0They tend to be=
pointless "*********** PUBLIC METHODS ***********" or they show t= hat the
code should really be split. =A0Just a pet peeve but please do split it if<= br> possible.


Did it.<= /div>

=A0
> > +static inline void vmxnet3_flush_shmem_changes(void)
> > +{
> > + =A0 =A0/*
> > + =A0 =A0 * Flush shared memory changes
> > + =A0 =A0 * Needed before sending interrupt to guest to ensure > > + =A0 =A0 * it gets consistent memory state
> > + =A0 =A0 */
> > + =A0 =A0smp_wmb();
> > +}
>
> It's useful to document why a memory barrier is being used in each=
> instance. =A0Therefore hiding smp_wmb() inside a wrapper function isn&= #39;t
> great.
>
> [DF] Fixed.
>
> Also, it's suspicious that smb_wmb() is used but no other barriers= are
> used. =A0What about a read memory barrier when accessing shared memory=
> written by the guest?
>
> [DF] VMWARE interface build a in safe way - you always read out data b= uffer (packet descriptor) with memcopy,
> [DF] and then check its last byte to see whether it was fully filled b= y driver.
> [DF] In this case device doesn't need read barriers. Drivers need = write barriers of course to secure
> [DF] proper order of writes.

That's not portable, you're making assumptions about memory o= rdering
which varies from architecture to architecture. =A0Some of the barriers
compile out on x86 because the memory model is strong.

QEMU is portable across host architectures so you need to use the read
memory barrier.

Stefan

Oh, I see,=A0thanks for explanation.
Fixed.


-= -
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman
--f46d042fd84687d0cb04d319caff--