xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@novell.com>
To: Steven Smith <steven.smith@citrix.com>, Steven.Smith@eu.citrix.com
Cc: Jeremy Fitzhardinge <jeremy@goop.org>,
	"xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	Dongxiao Xu <dongxiao.xu@intel.com>
Subject: Re: [Pv-ops][PATCH 0/3] Resend: Netback multiple thread support
Date: Fri, 30 Apr 2010 09:27:56 +0100	[thread overview]
Message-ID: <4BDAB0AC0200007800000CA1@vpn.id2.novell.com> (raw)
In-Reply-To: <20100430072926.GB22478@weybridge.uk.xensource.com>

>>> Steven Smith <steven.smith@citrix.com> 30.04.10 09:29 >>>
>It sounds like you've had a pretty good look at these patches.  Did
>you see anything else worth pointing out?

No, the major ones you had already pointed out.

Despite your earlier comments, even in the latest version the load
balancing code still doesn't seem right. I found it necessary to do
this right in __netif_{up,down}(), so that there is no potential of
mis-balanced increments and decrements (we also still have the
[pointless] list inserts/removes in our version of the patches,
which was what actually pointed out the issue by way of seeing
crashes or endless loops). This in turn made it necessary to add
logic to ignore the first (couple of?) invocation(s) of netif_be_int()
(i.e. before the netif as assigned to a group).

Also in spite of your earlier comments, the use of
kthread_should_stop() in the latest version of the patches still
seems insufficient - afaict it should also be used in the
expression passed to wait_event_interruptible().

Minor ones are
- should not use kzalloc() for allocating the (huge) array of struct
  xen_netbk in netback_init()
- the changes to netif_be_dbg() are bogus
- tasklets get replaced unconditionally with kthreads - I opted to
  make this dependent on a command line option, as the general
  description hinted both having their up- and downsides
- placement of fields in struct xen_netbk (I'd prefer all small fields to
  precede the large arrays - on x86 this results in smaller code)

Jan

  reply	other threads:[~2010-04-30  8:27 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-27  2:26 [Pv-ops][PATCH] Netback multiple tasklet support Xu, Dongxiao
2009-11-27  9:42 ` Ian Campbell
2009-11-27 16:08   ` Xu, Dongxiao
2009-11-27 16:15 ` Ian Pratt
2009-11-27 16:57   ` Xu, Dongxiao
2009-11-28 13:15     ` Ian Pratt
2009-12-02 10:17       ` Xu, Dongxiao
2009-12-03 21:28         ` Jeremy Fitzhardinge
2009-12-04  2:13           ` Xu, Dongxiao
2009-12-04  2:33             ` Jeremy Fitzhardinge
2009-12-08  9:22               ` Xu, Dongxiao
2009-12-09 20:23                 ` Jeremy Fitzhardinge
2009-12-10  3:29                   ` Xu, Dongxiao
2009-12-10 18:01                     ` Jeremy Fitzhardinge
2009-12-11  1:34                       ` Xu, Dongxiao
2010-04-26 14:27                       ` [Pv-ops][PATCH 0/3] Resend: Netback multiple thread support Xu, Dongxiao
2010-04-27  0:19                         ` Konrad Rzeszutek Wilk
2010-04-27  0:40                           ` Xu, Dongxiao
2010-04-27  3:02                         ` Xu, Dongxiao
2010-04-27 10:49                         ` Steven Smith
2010-04-27 18:37                           ` Jeremy Fitzhardinge
2010-04-28  9:31                             ` Steven Smith
2010-04-28 11:36                               ` Xu, Dongxiao
2010-04-28 12:04                                 ` Steven Smith
2010-04-28 13:33                                   ` Xu, Dongxiao
2010-04-30  7:35                                     ` Steven Smith
2010-04-28 10:27                           ` Xu, Dongxiao
2010-04-28 11:51                             ` Steven Smith
2010-04-28 12:23                               ` Xu, Dongxiao
2010-04-28 12:43                               ` Jan Beulich
2010-04-30  7:29                                 ` Steven Smith
2010-04-30  8:27                                   ` Jan Beulich [this message]
2009-12-10  9:07                   ` [Pv-ops][PATCH] Netback multiple tasklet support Ian Campbell
2009-12-10 17:54                     ` Jeremy Fitzhardinge
2009-12-10 18:07                       ` Ian Campbell
2009-12-11  8:34                         ` Jan Beulich
2009-12-11  9:34                           ` Ian Campbell
2009-12-11 14:24                             ` Konrad Rzeszutek Wilk
2010-03-17  8:46                       ` [PATCH] [pv-ops] fix dom0 S3 when MSI is used Cui, Dexuan
2010-03-17 14:28                         ` Konrad Rzeszutek Wilk
2010-03-18  3:05                           ` Cui, Dexuan
2010-03-19  1:04                           ` Jeremy Fitzhardinge
2010-03-19  1:03                         ` Jeremy Fitzhardinge
2010-03-19  1:29                           ` Cui, Dexuan
2010-01-13 10:17                     ` [Pv-ops][PATCH] Netback multiple tasklet support Jan Beulich
2010-01-14 16:55                       ` Ian Campbell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4BDAB0AC0200007800000CA1@vpn.id2.novell.com \
    --to=jbeulich@novell.com \
    --cc=Steven.Smith@eu.citrix.com \
    --cc=dongxiao.xu@intel.com \
    --cc=jeremy@goop.org \
    --cc=steven.smith@citrix.com \
    --cc=xen-devel@lists.xensource.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).