* Re: 0xB16B00B5? Really? (was Re: Move hyperv out of the drivers/staging/ directory)
From: valdis.kletnieks @ 2012-07-21 23:21 UTC (permalink / raw)
To: richard -rw- weinberger
Cc: KY Srinivasan, Bjørn Mork,
Greg KH (gregkh@linuxfoundation.org), Paolo Bonzini,
devel@linuxdriverproject.org, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org
In-Reply-To: <CAFLxGvwqKLhPWNfUJYm1KDXWEX1DNhPBGGApYFqMM0BsB87iig@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]
On Fri, 20 Jul 2012 17:03:59 +0200, richard -rw- weinberger said:
> On Fri, Jul 20, 2012 at 4:00 PM, KY Srinivasan <kys@microsoft.com> wrote:
> > Thank you for your interest in fixing this problem. When we decide to change this
> > ID, we will conform to the MSFT guidelines on constructing this guest ID.
> >
>
> I'm wondering why it hasn't been conform to the MSFT guidelines from
> the very beginning on?
It's a lot easier to sneak something cute into code when it isn't maintained in
git repositories and mailing lists readable by everybody and their pet llama.
I'll admit to not doing a *lot* of extensive review, but I try to at least read
Kconfig patches (mostly out of self-defense so when I do a 'make oldconfig' I
get presented with something useful ;) - and it's amazing how often I spot
issues in stuff that's presumably already had several pair of eyeballs look at
it already.
Probably what happened - some programmer had a 60 hour week, wrote the code on
a Friday and had a sudden attack of the sillies, and it went through a code
review meeting, but they had to cover 800 lines of code in a one-hour meeting
so nobody looked *too* close...
[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]
^ permalink raw reply
* Re: [PATCH 00/13] drivers: hv: kvp
From: Ben Hutchings @ 2012-07-22 2:50 UTC (permalink / raw)
To: KY Srinivasan
Cc: Olaf Hering, Greg KH, apw@canonical.com,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF9155EF7C6@SN2PRD0310MB382.namprd03.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]
On Tue, 2012-07-03 at 15:24 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Monday, July 02, 2012 3:57 PM
> > To: KY Srinivasan
> > Cc: Olaf Hering; Greg KH; apw@canonical.com; devel@linuxdriverproject.org;
> > virtualization@lists.osdl.org; linux-kernel@vger.kernel.org;
> > netdev@vger.kernel.org
> > Subject: Re: [PATCH 00/13] drivers: hv: kvp
> >
> > On Mon, Jul 02, 2012 at 03:22:25PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > > Sent: Thursday, June 28, 2012 10:24 AM
> > > > To: KY Srinivasan
> > > > Cc: Greg KH; apw@canonical.com; devel@linuxdriverproject.org;
> > > > virtualization@lists.osdl.org; linux-kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 00/13] drivers: hv: kvp
> > > >
> > > > On Tue, Jun 26, KY Srinivasan wrote:
> > > >
> > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > > > The fact that it was Red Hat specific was the main part, this should be
> > > > > > done in a standard way, with standard tools, right?
> > > > >
> > > > > The reason I asked this question was to make sure I address these
> > > > > issues in addition to whatever I am debugging now. I use the standard
> > > > > tools and calls to retrieve all the IP configuration. As I look at
> > > > > each distribution the files they keep persistent IP configuration
> > > > > Information is different and that is the reason I chose to start with
> > > > > RedHat. If there is a standard way to store the configuration, I will
> > > > > do that.
> > > >
> > > >
> > > > KY,
> > > >
> > > > instead of using system() in kvp_get_ipconfig_info and kvp_set_ip_info,
> > > > wouldnt it be easier to call an external helper script which does all
> > > > the distribution specific work? Just define some API to pass values to
> > > > the script, and something to read values collected by the script back
> > > > into the daemon.
> > >
> > > On the "Get" side I mostly use standard commands/APIs to get all the
> > information:
> > >
> > > 1) IP address information and subnet mask: getifaddrs()
> > > 2) DNS information: Parsing /etc/resolv.conf
> > > 3) /sbin/ip command for all the routing information
> >
> > If you're interested in the *current* configuration then (1) and (3)
> > are OK but you should really use the rtnetlink API.
> >
> > However, I suspect that Hyper-V assumes that current and persistent
> > configuration are the same thing, which is obviously not true in
> > general on Linux. But if NetworkManager is running then you can
> > assume they are.
>
> I am only interested in the currently active information. Why do you
> recommend the use of rtnetlink API over the "ip" command. If I am not
> mistaken, the ip command uses netlink to get the information.
[...]
'Screen-scraping' the output of administrative tools is not good
practice. It may be the best you can do when writing a shell script,
but for a C program it's generaly less reliable and often more difficult
than using the underlying C API.
Ben.
--
Ben Hutchings
73.46% of all statistics are made up.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* RE: [PATCH 00/13] drivers: hv: kvp
From: KY Srinivasan @ 2012-07-22 15:08 UTC (permalink / raw)
To: Ben Hutchings
Cc: Olaf Hering, Greg KH, apw@canonical.com,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <1342925458.11373.210.camel@deadeye.wl.decadent.org.uk>
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Saturday, July 21, 2012 10:51 PM
> To: KY Srinivasan
> Cc: Olaf Hering; Greg KH; apw@canonical.com; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH 00/13] drivers: hv: kvp
>
> On Tue, 2012-07-03 at 15:24 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > Sent: Monday, July 02, 2012 3:57 PM
> > > To: KY Srinivasan
> > > Cc: Olaf Hering; Greg KH; apw@canonical.com; devel@linuxdriverproject.org;
> > > virtualization@lists.osdl.org; linux-kernel@vger.kernel.org;
> > > netdev@vger.kernel.org
> > > Subject: Re: [PATCH 00/13] drivers: hv: kvp
> > >
> > > On Mon, Jul 02, 2012 at 03:22:25PM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > > > Sent: Thursday, June 28, 2012 10:24 AM
> > > > > To: KY Srinivasan
> > > > > Cc: Greg KH; apw@canonical.com; devel@linuxdriverproject.org;
> > > > > virtualization@lists.osdl.org; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH 00/13] drivers: hv: kvp
> > > > >
> > > > > On Tue, Jun 26, KY Srinivasan wrote:
> > > > >
> > > > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > > > > The fact that it was Red Hat specific was the main part, this should be
> > > > > > > done in a standard way, with standard tools, right?
> > > > > >
> > > > > > The reason I asked this question was to make sure I address these
> > > > > > issues in addition to whatever I am debugging now. I use the standard
> > > > > > tools and calls to retrieve all the IP configuration. As I look at
> > > > > > each distribution the files they keep persistent IP configuration
> > > > > > Information is different and that is the reason I chose to start with
> > > > > > RedHat. If there is a standard way to store the configuration, I will
> > > > > > do that.
> > > > >
> > > > >
> > > > > KY,
> > > > >
> > > > > instead of using system() in kvp_get_ipconfig_info and kvp_set_ip_info,
> > > > > wouldnt it be easier to call an external helper script which does all
> > > > > the distribution specific work? Just define some API to pass values to
> > > > > the script, and something to read values collected by the script back
> > > > > into the daemon.
> > > >
> > > > On the "Get" side I mostly use standard commands/APIs to get all the
> > > information:
> > > >
> > > > 1) IP address information and subnet mask: getifaddrs()
> > > > 2) DNS information: Parsing /etc/resolv.conf
> > > > 3) /sbin/ip command for all the routing information
> > >
> > > If you're interested in the *current* configuration then (1) and (3)
> > > are OK but you should really use the rtnetlink API.
> > >
> > > However, I suspect that Hyper-V assumes that current and persistent
> > > configuration are the same thing, which is obviously not true in
> > > general on Linux. But if NetworkManager is running then you can
> > > assume they are.
> >
> > I am only interested in the currently active information. Why do you
> > recommend the use of rtnetlink API over the "ip" command. If I am not
> > mistaken, the ip command uses netlink to get the information.
> [...]
>
> 'Screen-scraping' the output of administrative tools is not good
> practice. It may be the best you can do when writing a shell script,
> but for a C program it's generaly less reliable and often more difficult
> than using the underlying C API.
Ben,
Based on the input I have gotten, the consensus appears to be to have external
scripts to both GET and SET IP related configuration information. So, the KVP
daemon will need to parse information returned from these external distro specific
scripts (on the GET side). While I agree with you that it is good to use C APIs, I currently
have the implementation using the "ip" command and it appears to be quite simple.
Furthermore, given that the information I need to pass back to the host needs to be
appropriately formatted (based on host specified format), I suspect using the "ip" command
may actually simplify the code.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Minchan Kim @ 2012-07-23 2:33 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Andrew Morton,
Rafael Aquini
In-Reply-To: <20120720194858.GA16249@t510.redhat.com>
Hi Rafael,
On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> Howdy Minchan,
>
> Once again, thanks for raising such valuable feedback over here.
>
> On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > +static bool isolate_balloon_page(struct page *page)
> > > +{
> > > + if (WARN_ON(!is_balloon_page(page)))
> > > + return false;
> >
> > I am not sure we need this because you alreay check it before calling
> > isolate_balloon_page. If you really need it, it would be better to
> > add likely in isolate_balloon_page, too.
> >
>
> This check point was set in place because isolate_balloon_page() was a publicly
> visible function and while our current usage looks correct it would not hurt to
> have something like that done -- think of it as an insurance policy, in case
> someone else, in the future, attempts to use it on any other place outside this
> specifc context.
> Despite not seeing it as a dealbreaker for the patch as is, I do agree, however,
> this snippet can _potentially_ be removed from isolate_balloon_page(), since
> this function has become static to compaction.c.
Yes. It's not static.
>
>
> > > +
> > > + if (likely(get_page_unless_zero(page))) {
> > > + /*
> > > + * We can race against move_to_new_page() & __unmap_and_move().
> > > + * If we stumble across a locked balloon page and succeed on
> > > + * isolating it, the result tends to be disastrous.
> > > + */
> > > + if (likely(trylock_page(page))) {
> >
> > Hmm, I can't understand your comment.
> > What does this lock protect? Could you elaborate it with code sequence?
> >
>
> As we are coping with a corner case -- balloon pages are not on LRU lists --
> compaction concurrency can lead to a disastrous race which results in
> isolate_balloon_page() re-isolating already isolated balloon pages, or isolating
> a 'newpage' recently promoted to 'balloon page', while these pages are still
> under migration. The only way we have to prevent that from happening is
> attempting to grab the page lock, as pages under migration are already locked.
> I had that comment rephrased to what follows (please, tell me how it sounds to
> you now)
> if (likely(get_page_unless_zero(page))) {
> /*
> - * We can race against move_to_new_page() & __unmap_and_move().
> - * If we stumble across a locked balloon page and succeed on
> - * isolating it, the result tends to be disastrous.
> + * As balloon pages are not isolated from LRU lists, several
> + * concurrent compaction threads will potentially race against
> + * page migration at move_to_new_page() & __unmap_and_move().
> + * In order to avoid having an already isolated balloon page
> + * being (wrongly) re-isolated while it is under migration,
> + * lets be sure we have the page lock before proceeding with
> + * the balloon page isolation steps.
Looks good to me.
> */
> if (likely(trylock_page(page))) {
> /*
>
>
> > > +/* putback_lru_page() counterpart for a ballooned page */
> > > +bool putback_balloon_page(struct page *page)
> > > +{
> > > + if (WARN_ON(!is_balloon_page(page)))
> > > + return false;
> >
> > You already check WARN_ON in putback_lru_pages so we don't need it in here.
> > And you can add likely in here, too.
> >
>
> This check point is in place by the same reason why it is for
> isolate_balloon_page(). However, I don't think we should drop it here because
> putback_balloon_page() is still a publicly visible symbol. Besides, the check
> that is performed at putback_lru_pages() level has a different meaning, which is
> to warn us when we fail on re-inserting an isolated (but not migrated) page back
> to the balloon page list, thus it does not superceeds nor it's superceeded by
> this checkpoint here.
I'm not against you strongly but IMHO, putback_balloon_page is never generic
function which is likely to be used by many others so I hope not overdesign.
>
>
> > > + } else if (is_balloon_page(page)) {
> >
> > unlikely?
>
> This can be done, for sure. Also, it reminds me that I had a
> 'likely(PageLRU(page))' set in place for this vary same patch, on v2 submission.
> Do you recollect you've asked me to get rid of it?. Wouldn't it be better having
> that suggestion of yours reverted, since PageLRU(page) is the likelihood case
> here anyway? What about this?
>
> " if (likely(PageLRU(page))) {
> ...
> } else if (unlikely(is_balloon_page(page))) {
> ...
> } else
> continue;
> "
I don't like that.
PageLRU isn't likelihood case in this.
>
> >
> > > @@ -78,7 +78,10 @@ void putback_lru_pages(struct list_head *l)
> > > list_del(&page->lru);
> > > dec_zone_page_state(page, NR_ISOLATED_ANON +
> > > page_is_file_cache(page));
> > > - putback_lru_page(page);
> > > + if (unlikely(is_balloon_page(page)))
> > > + WARN_ON(!putback_balloon_page(page));
> > > + else
> > > + putback_lru_page(page);
> >
> > Hmm, I don't like this.
> > The function name says we putback *lru* pages, but balloon page isn't.
> > How about this?
> >
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index aad0a16..b07cd67 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
> > * Apart from cc->migratepages and cc->nr_migratetypes this function
> > * does not modify any cc's fields, in particular it does not modify
> > * (or read for that matter) cc->migrate_pfn.
> > + *
> > + * For returning page, you should use putback_pages instead of putback_lru_pages
> > */
> > unsigned long
> > isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> > @@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >
> > /* Release LRU pages not migrated */
> > if (err) {
> > - putback_lru_pages(&cc->migratepages);
> > + putback_pages(&cc->migratepages);
> > cc->nr_migratepages = 0;
> > if (err == -ENOMEM) {
> > ret = COMPACT_PARTIAL;
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 9705e70..a96b840 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
> > }
> > }
> >
> > + /* blah blah .... */
> > +void putback_pages(struct list_head *l)
> > +{
> > + struct page *page;
> > + struct page *page2;
> > +
> > + list_for_each_entry_safe(page, page2, l, lru) {
> > + list_del(&page->lru);
> > + dec_zone_page_state(page, NR_ISOLATED_ANON +
> > + page_is_file_cache(page));
> > + if (unlikely(is_balloon_page(page)))
> > + WARN_ON(!putback_balloon_page(page));
> > + else
> > + putback_lru_page(page);
> > + }
> > +}
> > +
> > /*
> > * Restore a potential migration pte to a working pte entry
> > */
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 32985dd..decb82a 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> > 0, false, MIGRATE_SYNC);
> > }
> >
> > - putback_lru_pages(&cc.migratepages);
> > + putback_pages(&cc.migratepages);
> > return ret > 0 ? 0 : ret;
> > }
> >
>
> Despite being a very nice cleanup, this code refactoring has nothing to do with
> this particular patch purpose. For the sake of this implementation, think about
> the balloon page list acting as a particular LRU, so although ballooned pages
> are not enlisted on real LRUs, per se, this doesn't mean we cannot have them
> dealt as a corner case amongst putback_lru_pages() code for the sake of
> simplicity and maintainability ease. OTOH, I do agree with your nice suggestion,
> thus I can submit it as a separate clean-up patch attached to this series (if
> you don't mind).
No problem. But the concern is that I can't agree this patch implementation.
Please see below.
>
>
> > >
> > > + if (is_balloon_page(page)) {
> >
> > unlikely?
> >
>
> Yeah, why not? Will do it.
>
> > > + if (is_balloon_page(newpage)) {
> >
> > unlikely?
> >
>
> ditto.
>
> > > + /*
> > > + * A ballooned page has been migrated already. Now, it is the
> > > + * time to wrap-up counters, handle the old page back to Buddy
> > > + * and return.
> > > + */
> > > + list_del(&page->lru);
> > > + dec_zone_page_state(page, NR_ISOLATED_ANON +
> > > + page_is_file_cache(page));
> > > + put_page(page);
> > > + __free_page(page);
> >
> > Why do you use __free_page instead of put_page?
> >
>
> Do you mean perform an extra put_page() and let putback_lru_page() do the rest?
> Besides looking odd at code level, what other improvement we can potentially gain here?
I mean just do
put_page(page); /* blah blah */
put_page(page); /* It will free the page to buddy */
Yeb it's more fat than __free_page for your case. If so, please comment write out
why you use __free_page instead of put_page.
/*
* balloon page should be never compund page and not on the LRU so we
* call __free_page directly instead of put_page.
*/
>
>
> > The feeling I look at your code in detail is that it makes compaction/migration
> > code rather complicated because compaction/migration assumes source/target would
> > be LRU pages.
> >
> > How often memory ballooning happens? Does it make sense to hook it in generic
> > functions if it's very rare?
> >
> > Couldn't you implement it like huge page?
> > It doesn't make current code complicated and doesn't affect performance
> >
> > In compaction,
> > #ifdef CONFIG_VIRTIO_BALLOON
> > static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)
> > {
> > if (balloon) {
> > isolate_balloonpages
> >
> > migrate_balloon_pages
> > unmap_and_move_balloon_page
> >
> > putback_balloonpages
> > }
> > }
> > #endif
> >
> > I'm not sure memory ballooning so it might be dumb question.
> > Can we trigger it once only when ballooning happens?
>
> I strongly believe, this is the simplest and easiest way to get this task
> accomplished, mostly becasue:
> i. It does not require any code duplication at all;
> ii. It requires very few code insertion/surgery to be fully operative;
> iii. It is embeded on already well established and maintained code;
> iv. The approach makes easier to other balloon drivers leverage compaction
> code;
> v. It shows no significative impact to the entangled code paths;
But it is adding a few hooks to generic functions which all assume they
are LRU pages. Although it's trivial for performance, we don't need get
such loss due to very infrequent event.
More concern to me that it makes code complicated because we assume
functions related migration/compaction depend on LRU pages.
Let me say a example.
suitable_migration_target in isolate_freepages returns true pageblock
type is MIGRATE_MOVABLE or MIGRATE_CMA which expect pages in that block
is migratable so they should be moved into another location or reclaimed
if we have no space for getting big contiguos space.
All of such code should consider balloon page but balloon page can't be
reclaimed unlike normal LRU page so it makes design more complex.
Look at memory-hotplug, offline_page calls has_unmovable_pages, scan_lru_pages
and do_migrate_range which calls isolate_lru_page. They consider only LRU pages
to migratable ones.
IMHO, better approach is that after we can get complete free pageblocks
by compaction or reclaim, move balloon pages into that pageblocks and make
that blocks to unmovable. It can prevent fragmentation and it makes
current or future code don't need to consider balloon page.
Of course, it needs more code but I think it's valuable.
What do you think about it?
--
Kind regards,
Minchan Kim
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Minchan Kim @ 2012-07-23 2:35 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Andrew Morton,
Rafael Aquini
In-Reply-To: <20120723023332.GA6832@bbox>
On Mon, Jul 23, 2012 at 11:33:32AM +0900, Minchan Kim wrote:
> Hi Rafael,
>
> On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> > Howdy Minchan,
> >
> > Once again, thanks for raising such valuable feedback over here.
> >
> > On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > > +static bool isolate_balloon_page(struct page *page)
> > > > +{
> > > > + if (WARN_ON(!is_balloon_page(page)))
> > > > + return false;
> > >
> > > I am not sure we need this because you alreay check it before calling
> > > isolate_balloon_page. If you really need it, it would be better to
> > > add likely in isolate_balloon_page, too.
> > >
> >
> > This check point was set in place because isolate_balloon_page() was a publicly
> > visible function and while our current usage looks correct it would not hurt to
> > have something like that done -- think of it as an insurance policy, in case
> > someone else, in the future, attempts to use it on any other place outside this
> > specifc context.
> > Despite not seeing it as a dealbreaker for the patch as is, I do agree, however,
> > this snippet can _potentially_ be removed from isolate_balloon_page(), since
> > this function has become static to compaction.c.
>
> Yes. It's not static.
Typo. It's static.
--
Kind regards,
Minchan Kim
^ permalink raw reply
* Re: [net-next RFC V5 5/5] virtio_net: support negotiating the number of queues through ctrl vq
From: Jason Wang @ 2012-07-23 5:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, mashirle, kvm, netdev, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120720123320.GC16550@redhat.com>
On 07/20/2012 08:33 PM, Michael S. Tsirkin wrote:
> On Thu, Jul 05, 2012 at 06:29:54PM +0800, Jason Wang wrote:
>> This patch let the virtio_net driver can negotiate the number of queues it
>> wishes to use through control virtqueue and export an ethtool interface to let
>> use tweak it.
>>
>> As current multiqueue virtio-net implementation has optimizations on per-cpu
>> virtuqueues, so only two modes were support:
>>
>> - single queue pair mode
>> - multiple queue paris mode, the number of queues matches the number of vcpus
>>
>> The single queue mode were used by default currently due to regression of
>> multiqueue mode in some test (especially in stream test).
>>
>> Since virtio core does not support paritially deleting virtqueues, so during
>> mode switching the whole virtqueue were deleted and the driver would re-create
>> the virtqueues it would used.
>>
>> btw. The queue number negotiating were defered to .ndo_open(), this is because
>> only after feature negotitaion could we send the command to control virtqueue
>> (as it may also use event index).
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 171 ++++++++++++++++++++++++++++++++++---------
>> include/linux/virtio_net.h | 7 ++
>> 2 files changed, 142 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 7410187..3339eeb 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -88,6 +88,7 @@ struct receive_queue {
>>
>> struct virtnet_info {
>> u16 num_queue_pairs; /* # of RX/TX vq pairs */
>> + u16 total_queue_pairs;
>>
>> struct send_queue *sq[MAX_QUEUES] ____cacheline_aligned_in_smp;
>> struct receive_queue *rq[MAX_QUEUES] ____cacheline_aligned_in_smp;
>> @@ -137,6 +138,8 @@ struct padded_vnet_hdr {
>> char padding[6];
>> };
>>
>> +static const struct ethtool_ops virtnet_ethtool_ops;
>> +
>> static inline int txq_get_qnum(struct virtnet_info *vi, struct virtqueue *vq)
>> {
>> int ret = virtqueue_get_queue_index(vq);
>> @@ -802,22 +805,6 @@ static void virtnet_netpoll(struct net_device *dev)
>> }
>> #endif
>>
>> -static int virtnet_open(struct net_device *dev)
>> -{
>> - struct virtnet_info *vi = netdev_priv(dev);
>> - int i;
>> -
>> - for (i = 0; i< vi->num_queue_pairs; i++) {
>> - /* Make sure we have some buffers: if oom use wq. */
>> - if (!try_fill_recv(vi->rq[i], GFP_KERNEL))
>> - queue_delayed_work(system_nrt_wq,
>> - &vi->rq[i]->refill, 0);
>> - virtnet_napi_enable(vi->rq[i]);
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * Send command via the control virtqueue and check status. Commands
>> * supported by the hypervisor, as indicated by feature bits, should
>> @@ -873,6 +860,43 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>> rtnl_unlock();
>> }
>>
>> +static int virtnet_set_queues(struct virtnet_info *vi)
>> +{
>> + struct scatterlist sg;
>> + struct net_device *dev = vi->dev;
>> + sg_init_one(&sg,&vi->num_queue_pairs, sizeof(vi->num_queue_pairs));
>> +
>> + if (!vi->has_cvq)
>> + return -EINVAL;
>> +
>> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MULTIQUEUE,
>> + VIRTIO_NET_CTRL_MULTIQUEUE_QNUM,&sg, 1, 0)){
>> + dev_warn(&dev->dev, "Fail to set the number of queue pairs to"
>> + " %d\n", vi->num_queue_pairs);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_open(struct net_device *dev)
>> +{
>> + struct virtnet_info *vi = netdev_priv(dev);
>> + int i;
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + /* Make sure we have some buffers: if oom use wq. */
>> + if (!try_fill_recv(vi->rq[i], GFP_KERNEL))
>> + queue_delayed_work(system_nrt_wq,
>> + &vi->rq[i]->refill, 0);
>> + virtnet_napi_enable(vi->rq[i]);
>> + }
>> +
>> + virtnet_set_queues(vi);
>> +
>> + return 0;
>> +}
>> +
>> static int virtnet_close(struct net_device *dev)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> @@ -1013,12 +1037,6 @@ static void virtnet_get_drvinfo(struct net_device *dev,
>>
>> }
>>
>> -static const struct ethtool_ops virtnet_ethtool_ops = {
>> - .get_drvinfo = virtnet_get_drvinfo,
>> - .get_link = ethtool_op_get_link,
>> - .get_ringparam = virtnet_get_ringparam,
>> -};
>> -
>> #define MIN_MTU 68
>> #define MAX_MTU 65535
>>
>> @@ -1235,7 +1253,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>
>> err:
>> if (ret&& names)
>> - for (i = 0; i< vi->num_queue_pairs * 2; i++)
>> + for (i = 0; i< total_vqs * 2; i++)
>> kfree(names[i]);
>>
>> kfree(names);
>> @@ -1373,7 +1391,6 @@ static int virtnet_probe(struct virtio_device *vdev)
>> mutex_init(&vi->config_lock);
>> vi->config_enable = true;
>> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
>> - vi->num_queue_pairs = num_queue_pairs;
>>
>> /* If we can receive ANY GSO packets, we must allocate large ones. */
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> @@ -1387,6 +1404,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>> vi->has_cvq = true;
>>
>> + /* Use single tx/rx queue pair as default */
>> + vi->num_queue_pairs = 1;
>> + vi->total_queue_pairs = num_queue_pairs;
>> +
>> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
>> err = virtnet_setup_vqs(vi);
>> if (err)
>> @@ -1396,6 +1417,9 @@ static int virtnet_probe(struct virtio_device *vdev)
>> virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
>> dev->features |= NETIF_F_HW_VLAN_FILTER;
>>
>> + netif_set_real_num_tx_queues(dev, 1);
>> + netif_set_real_num_rx_queues(dev, 1);
>> +
>> err = register_netdev(dev);
>> if (err) {
>> pr_debug("virtio_net: registering device failed\n");
>> @@ -1403,7 +1427,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>> }
>>
>> /* Last of all, set up some receive buffers. */
>> - for (i = 0; i< num_queue_pairs; i++) {
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> try_fill_recv(vi->rq[i], GFP_KERNEL);
>>
>> /* If we didn't even get one input buffer, we're useless. */
>> @@ -1474,10 +1498,8 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
>> free_netdev(vi->dev);
>> }
>>
>> -#ifdef CONFIG_PM
>> -static int virtnet_freeze(struct virtio_device *vdev)
>> +static void virtnet_stop(struct virtnet_info *vi)
>> {
>> - struct virtnet_info *vi = vdev->priv;
>> int i;
>>
>> /* Prevent config work handler from accessing the device */
>> @@ -1493,17 +1515,10 @@ static int virtnet_freeze(struct virtio_device *vdev)
>> for (i = 0; i< vi->num_queue_pairs; i++)
>> napi_disable(&vi->rq[i]->napi);
>>
>> -
>> - remove_vq_common(vi);
>> -
>> - flush_work(&vi->config_work);
>> -
>> - return 0;
>> }
>>
>> -static int virtnet_restore(struct virtio_device *vdev)
>> +static int virtnet_start(struct virtnet_info *vi)
>> {
>> - struct virtnet_info *vi = vdev->priv;
>> int err, i;
>>
>> err = virtnet_setup_vqs(vi);
>> @@ -1527,6 +1542,29 @@ static int virtnet_restore(struct virtio_device *vdev)
>>
>> return 0;
>> }
>> +
>> +#ifdef CONFIG_PM
>> +static int virtnet_freeze(struct virtio_device *vdev)
>> +{
>> + struct virtnet_info *vi = vdev->priv;
>> +
>> + virtnet_stop(vi);
>> +
>> + remove_vq_common(vi);
>> +
>> + flush_work(&vi->config_work);
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_restore(struct virtio_device *vdev)
>> +{
>> + struct virtnet_info *vi = vdev->priv;
>> +
>> + virtnet_start(vi);
>> +
>> + return 0;
>> +}
>> #endif
>>
>> static struct virtio_device_id id_table[] = {
>> @@ -1560,6 +1598,67 @@ static struct virtio_driver virtio_net_driver = {
>> #endif
>> };
>>
>> +static int virtnet_set_channels(struct net_device *dev,
>> + struct ethtool_channels *channels)
>> +{
>> + struct virtnet_info *vi = netdev_priv(dev);
>> + u16 queues = channels->rx_count;
>> + unsigned status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
>> +
>> + if (channels->rx_count != channels->tx_count)
>> + return -EINVAL;
>> + /* Only two modes were support currently */
> s/were/are/ ?
Ok.
>
>> + if (queues != vi->total_queue_pairs&& queues != 1)
>> + return -EINVAL;
> So userspace has to get queue number right. How does it know
> what the valid value is?
Usespace could query the number through ethtool -l (virtnet_get_channels()).
>
>> + if (!vi->has_cvq)
>> + return -EINVAL;
>> +
>> + virtnet_stop(vi);
>> +
>> + netif_set_real_num_tx_queues(dev, queues);
>> + netif_set_real_num_rx_queues(dev, queues);
>> +
>> + remove_vq_common(vi);
>> + flush_work(&vi->config_work);
>> +
>> + vi->num_queue_pairs = queues;
>> + virtnet_start(vi);
>> +
>> + vi->vdev->config->finalize_features(vi->vdev);
>> +
>> + if (virtnet_set_queues(vi))
>> + status |= VIRTIO_CONFIG_S_FAILED;
>> + else
>> + status |= VIRTIO_CONFIG_S_DRIVER_OK;
>> +
>> + vi->vdev->config->set_status(vi->vdev, status);
>> +
> Why do we need to tweak status like that?
Because remove_vq_common() reset the device. Since virtio core api does
not support remove a specified number of virtqueues, it's the only
method when we change the number of queues.
> Can we maybe just roll changes back on error?
Not easy, we reset and detroy previous virtqueues and create new ones.
>> + return 0;
>> +}
>> +
>> +static void virtnet_get_channels(struct net_device *dev,
>> + struct ethtool_channels *channels)
>> +{
>> + struct virtnet_info *vi = netdev_priv(dev);
>> +
>> + channels->max_rx = vi->total_queue_pairs;
>> + channels->max_tx = vi->total_queue_pairs;
>> + channels->max_other = 0;
>> + channels->max_combined = 0;
>> + channels->rx_count = vi->num_queue_pairs;
>> + channels->tx_count = vi->num_queue_pairs;
>> + channels->other_count = 0;
>> + channels->combined_count = 0;
>> +}
>> +
>> +static const struct ethtool_ops virtnet_ethtool_ops = {
>> + .get_drvinfo = virtnet_get_drvinfo,
>> + .get_link = ethtool_op_get_link,
>> + .get_ringparam = virtnet_get_ringparam,
>> + .set_channels = virtnet_set_channels,
>> + .get_channels = virtnet_get_channels,
>> +};
>> +
>> static int __init init(void)
>> {
>> return register_virtio_driver(&virtio_net_driver);
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 60f09ff..0d21e08 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -169,4 +169,11 @@ struct virtio_net_ctrl_mac {
>> #define VIRTIO_NET_CTRL_ANNOUNCE 3
>> #define VIRTIO_NET_CTRL_ANNOUNCE_ACK 0
>>
>> +/*
>> + * Control multiqueue
>> + *
>> + */
>> +#define VIRTIO_NET_CTRL_MULTIQUEUE 4
>> + #define VIRTIO_NET_CTRL_MULTIQUEUE_QNUM 0
>> +
>> #endif /* _LINUX_VIRTIO_NET_H */
>> --
>> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Jason Wang @ 2012-07-23 5:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, mashirle, kvm, netdev, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120720134014.GD16550@redhat.com>
On 07/20/2012 09:40 PM, Michael S. Tsirkin wrote:
> On Thu, Jul 05, 2012 at 06:29:53PM +0800, Jason Wang wrote:
>> This patch converts virtio_net to a multi queue device. After negotiated
>> VIRTIO_NET_F_MULTIQUEUE feature, the virtio device has many tx/rx queue pairs,
>> and driver could read the number from config space.
>>
>> The driver expects the number of rx/tx queue paris is equal to the number of
>> vcpus. To maximize the performance under this per-cpu rx/tx queue pairs, some
>> optimization were introduced:
>>
>> - Txq selection is based on the processor id in order to avoid contending a lock
>> whose owner may exits to host.
>> - Since the txq/txq were per-cpu, affinity hint were set to the cpu that owns
>> the queue pairs.
>>
>> Signed-off-by: Krishna Kumar<krkumar2@in.ibm.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Overall fine. I think it is best to smash the following patch
> into this one, so that default behavior does not
> jump to mq then back. some comments below: mostly nits, and a minor bug.
Sure, thanks for the reviewing.
>
> If you are worried the patch is too big, it can be split
> differently
> - rework to use send_queue/receive_queue structures, no
> functional changes.
> - add multiqueue
>
> but this is not a must.
>
>> ---
>> drivers/net/virtio_net.c | 645 ++++++++++++++++++++++++++++++-------------
>> include/linux/virtio_net.h | 2 +
>> 2 files changed, 452 insertions(+), 195 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1db445b..7410187 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -26,6 +26,7 @@
>> #include<linux/scatterlist.h>
>> #include<linux/if_vlan.h>
>> #include<linux/slab.h>
>> +#include<linux/interrupt.h>
>>
>> static int napi_weight = 128;
>> module_param(napi_weight, int, 0444);
>> @@ -41,6 +42,8 @@ module_param(gso, bool, 0444);
>> #define VIRTNET_SEND_COMMAND_SG_MAX 2
>> #define VIRTNET_DRIVER_VERSION "1.0.0"
>>
>> +#define MAX_QUEUES 256
>> +
>> struct virtnet_stats {
>> struct u64_stats_sync tx_syncp;
>> struct u64_stats_sync rx_syncp;
> Would be a bit better not to have artificial limits like that.
> Maybe allocate arrays at probe time, then we can
> take whatever the device gives us?
Sure.
>> @@ -51,43 +54,69 @@ struct virtnet_stats {
>> u64 rx_packets;
>> };
>>
>> -struct virtnet_info {
>> - struct virtio_device *vdev;
>> - struct virtqueue *rvq, *svq, *cvq;
>> - struct net_device *dev;
>> +/* Internal representation of a send virtqueue */
>> +struct send_queue {
>> + /* Virtqueue associated with this send _queue */
>> + struct virtqueue *vq;
>> +
>> + /* TX: fragments + linear part + virtio header */
>> + struct scatterlist sg[MAX_SKB_FRAGS + 2];
>> +};
>> +
>> +/* Internal representation of a receive virtqueue */
>> +struct receive_queue {
>> + /* Virtqueue associated with this receive_queue */
>> + struct virtqueue *vq;
>> +
>> + /* Back pointer to the virtnet_info */
>> + struct virtnet_info *vi;
>> +
>> struct napi_struct napi;
>> - unsigned int status;
>>
>> /* Number of input buffers, and max we've ever had. */
>> unsigned int num, max;
>>
>> + /* Work struct for refilling if we run low on memory. */
>> + struct delayed_work refill;
>> +
>> + /* Chain pages by the private ptr. */
>> + struct page *pages;
>> +
>> + /* RX: fragments + linear part + virtio header */
>> + struct scatterlist sg[MAX_SKB_FRAGS + 2];
>> +};
>> +
>> +struct virtnet_info {
>> + u16 num_queue_pairs; /* # of RX/TX vq pairs */
>> +
>> + struct send_queue *sq[MAX_QUEUES] ____cacheline_aligned_in_smp;
>> + struct receive_queue *rq[MAX_QUEUES] ____cacheline_aligned_in_smp;
> The assumption is a tx/rx pair is handled on the same cpu, yes?
> If yes maybe make it a single array to improve cache locality
> a bit?
> struct queue_pair {
> struct send_queue sq;
> struct receive_queue rq;
> };
Ok.
>> + struct virtqueue *cvq;
>> +
>> + struct virtio_device *vdev;
>> + struct net_device *dev;
>> + unsigned int status;
>> +
>> /* I like... big packets and I cannot lie! */
>> bool big_packets;
>>
>> /* Host will merge rx buffers for big packets (shake it! shake it!) */
>> bool mergeable_rx_bufs;
>>
>> + /* Has control virtqueue */
>> + bool has_cvq;
>> +
> won't checking (cvq != NULL) be enough?
Enough, so has_cvq is dupliated with vi->cvq. I will remove it in next
version.
>
>> /* enable config space updates */
>> bool config_enable;
>>
>> /* Active statistics */
>> struct virtnet_stats __percpu *stats;
>>
>> - /* Work struct for refilling if we run low on memory. */
>> - struct delayed_work refill;
>> -
>> /* Work struct for config space updates */
>> struct work_struct config_work;
>>
>> /* Lock for config space updates */
>> struct mutex config_lock;
>> -
>> - /* Chain pages by the private ptr. */
>> - struct page *pages;
>> -
>> - /* fragments + linear part + virtio header */
>> - struct scatterlist rx_sg[MAX_SKB_FRAGS + 2];
>> - struct scatterlist tx_sg[MAX_SKB_FRAGS + 2];
>> };
>>
>> struct skb_vnet_hdr {
>> @@ -108,6 +137,22 @@ struct padded_vnet_hdr {
>> char padding[6];
>> };
>>
>> +static inline int txq_get_qnum(struct virtnet_info *vi, struct virtqueue *vq)
>> +{
>> + int ret = virtqueue_get_queue_index(vq);
>> +
>> + /* skip ctrl vq */
>> + if (vi->has_cvq)
>> + return (ret - 1) / 2;
>> + else
>> + return ret / 2;
>> +}
>> +
>> +static inline int rxq_get_qnum(struct virtnet_info *vi, struct virtqueue *vq)
>> +{
>> + return virtqueue_get_queue_index(vq) / 2;
>> +}
>> +
>> static inline struct skb_vnet_hdr *skb_vnet_hdr(struct sk_buff *skb)
>> {
>> return (struct skb_vnet_hdr *)skb->cb;
>> @@ -117,22 +162,22 @@ static inline struct skb_vnet_hdr *skb_vnet_hdr(struct sk_buff *skb)
>> * private is used to chain pages for big packets, put the whole
>> * most recent used list in the beginning for reuse
>> */
>> -static void give_pages(struct virtnet_info *vi, struct page *page)
>> +static void give_pages(struct receive_queue *rq, struct page *page)
>> {
>> struct page *end;
>>
>> /* Find end of list, sew whole thing into vi->pages. */
>> for (end = page; end->private; end = (struct page *)end->private);
>> - end->private = (unsigned long)vi->pages;
>> - vi->pages = page;
>> + end->private = (unsigned long)rq->pages;
>> + rq->pages = page;
>> }
>>
>> -static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask)
>> +static struct page *get_a_page(struct receive_queue *rq, gfp_t gfp_mask)
>> {
>> - struct page *p = vi->pages;
>> + struct page *p = rq->pages;
>>
>> if (p) {
>> - vi->pages = (struct page *)p->private;
>> + rq->pages = (struct page *)p->private;
>> /* clear private here, it is used to chain pages */
>> p->private = 0;
>> } else
>> @@ -140,15 +185,15 @@ static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask)
>> return p;
>> }
>>
>> -static void skb_xmit_done(struct virtqueue *svq)
>> +static void skb_xmit_done(struct virtqueue *vq)
>> {
>> - struct virtnet_info *vi = svq->vdev->priv;
>> + struct virtnet_info *vi = vq->vdev->priv;
>>
>> /* Suppress further interrupts. */
>> - virtqueue_disable_cb(svq);
>> + virtqueue_disable_cb(vq);
>>
>> /* We were probably waiting for more output buffers. */
>> - netif_wake_queue(vi->dev);
>> + netif_wake_subqueue(vi->dev, txq_get_qnum(vi, vq));
>> }
>>
>> static void set_skb_frag(struct sk_buff *skb, struct page *page,
>> @@ -167,9 +212,10 @@ static void set_skb_frag(struct sk_buff *skb, struct page *page,
>> }
>>
>> /* Called from bottom half context */
>> -static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> +static struct sk_buff *page_to_skb(struct receive_queue *rq,
>> struct page *page, unsigned int len)
>> {
>> + struct virtnet_info *vi = rq->vi;
>> struct sk_buff *skb;
>> struct skb_vnet_hdr *hdr;
>> unsigned int copy, hdr_len, offset;
>> @@ -225,12 +271,12 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> }
>>
>> if (page)
>> - give_pages(vi, page);
>> + give_pages(rq, page);
>>
>> return skb;
>> }
>>
>> -static int receive_mergeable(struct virtnet_info *vi, struct sk_buff *skb)
>> +static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
>> {
>> struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
>> struct page *page;
>> @@ -244,7 +290,7 @@ static int receive_mergeable(struct virtnet_info *vi, struct sk_buff *skb)
>> skb->dev->stats.rx_length_errors++;
>> return -EINVAL;
>> }
>> - page = virtqueue_get_buf(vi->rvq,&len);
>> + page = virtqueue_get_buf(rq->vq,&len);
>> if (!page) {
>> pr_debug("%s: rx error: %d buffers missing\n",
>> skb->dev->name, hdr->mhdr.num_buffers);
>> @@ -257,13 +303,14 @@ static int receive_mergeable(struct virtnet_info *vi, struct sk_buff *skb)
>>
>> set_skb_frag(skb, page, 0,&len);
>>
>> - --vi->num;
>> + --rq->num;
>> }
>> return 0;
>> }
>>
>> -static void receive_buf(struct net_device *dev, void *buf, unsigned int len)
>> +static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
>> {
>> + struct net_device *dev = rq->vi->dev;
>> struct virtnet_info *vi = netdev_priv(dev);
>> struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>> struct sk_buff *skb;
>> @@ -274,7 +321,7 @@ static void receive_buf(struct net_device *dev, void *buf, unsigned int len)
>> pr_debug("%s: short packet %i\n", dev->name, len);
>> dev->stats.rx_length_errors++;
>> if (vi->mergeable_rx_bufs || vi->big_packets)
>> - give_pages(vi, buf);
>> + give_pages(rq, buf);
>> else
>> dev_kfree_skb(buf);
>> return;
>> @@ -286,14 +333,14 @@ static void receive_buf(struct net_device *dev, void *buf, unsigned int len)
>> skb_trim(skb, len);
>> } else {
>> page = buf;
>> - skb = page_to_skb(vi, page, len);
>> + skb = page_to_skb(rq, page, len);
>> if (unlikely(!skb)) {
>> dev->stats.rx_dropped++;
>> - give_pages(vi, page);
>> + give_pages(rq, page);
>> return;
>> }
>> if (vi->mergeable_rx_bufs)
>> - if (receive_mergeable(vi, skb)) {
>> + if (receive_mergeable(rq, skb)) {
>> dev_kfree_skb(skb);
>> return;
>> }
>> @@ -363,90 +410,91 @@ frame_err:
>> dev_kfree_skb(skb);
>> }
>>
>> -static int add_recvbuf_small(struct virtnet_info *vi, gfp_t gfp)
>> +static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
>> {
>> struct sk_buff *skb;
>> struct skb_vnet_hdr *hdr;
>> int err;
>>
>> - skb = __netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN, gfp);
>> + skb = __netdev_alloc_skb_ip_align(rq->vi->dev, MAX_PACKET_LEN, gfp);
>> if (unlikely(!skb))
>> return -ENOMEM;
>>
>> skb_put(skb, MAX_PACKET_LEN);
>>
>> hdr = skb_vnet_hdr(skb);
>> - sg_set_buf(vi->rx_sg,&hdr->hdr, sizeof hdr->hdr);
>> + sg_set_buf(rq->sg,&hdr->hdr, sizeof hdr->hdr);
>> +
>> + skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
>>
>> - skb_to_sgvec(skb, vi->rx_sg + 1, 0, skb->len);
>> + err = virtqueue_add_buf(rq->vq, rq->sg, 0, 2, skb, gfp);
>>
>> - err = virtqueue_add_buf(vi->rvq, vi->rx_sg, 0, 2, skb, gfp);
>> if (err< 0)
>> dev_kfree_skb(skb);
>>
>> return err;
>> }
>>
>> -static int add_recvbuf_big(struct virtnet_info *vi, gfp_t gfp)
>> +static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
>> {
>> struct page *first, *list = NULL;
>> char *p;
>> int i, err, offset;
>>
>> - /* page in vi->rx_sg[MAX_SKB_FRAGS + 1] is list tail */
>> + /* page in rq->sg[MAX_SKB_FRAGS + 1] is list tail */
>> for (i = MAX_SKB_FRAGS + 1; i> 1; --i) {
>> - first = get_a_page(vi, gfp);
>> + first = get_a_page(rq, gfp);
>> if (!first) {
>> if (list)
>> - give_pages(vi, list);
>> + give_pages(rq, list);
>> return -ENOMEM;
>> }
>> - sg_set_buf(&vi->rx_sg[i], page_address(first), PAGE_SIZE);
>> + sg_set_buf(&rq->sg[i], page_address(first), PAGE_SIZE);
>>
>> /* chain new page in list head to match sg */
>> first->private = (unsigned long)list;
>> list = first;
>> }
>>
>> - first = get_a_page(vi, gfp);
>> + first = get_a_page(rq, gfp);
>> if (!first) {
>> - give_pages(vi, list);
>> + give_pages(rq, list);
>> return -ENOMEM;
>> }
>> p = page_address(first);
>>
>> - /* vi->rx_sg[0], vi->rx_sg[1] share the same page */
>> - /* a separated vi->rx_sg[0] for virtio_net_hdr only due to QEMU bug */
>> - sg_set_buf(&vi->rx_sg[0], p, sizeof(struct virtio_net_hdr));
>> + /* rq->sg[0], rq->sg[1] share the same page */
>> + /* a separated rq->sg[0] for virtio_net_hdr only due to QEMU bug */
>> + sg_set_buf(&rq->sg[0], p, sizeof(struct virtio_net_hdr));
>>
>> - /* vi->rx_sg[1] for data packet, from offset */
>> + /* rq->sg[1] for data packet, from offset */
>> offset = sizeof(struct padded_vnet_hdr);
>> - sg_set_buf(&vi->rx_sg[1], p + offset, PAGE_SIZE - offset);
>> + sg_set_buf(&rq->sg[1], p + offset, PAGE_SIZE - offset);
>>
>> /* chain first in list head */
>> first->private = (unsigned long)list;
>> - err = virtqueue_add_buf(vi->rvq, vi->rx_sg, 0, MAX_SKB_FRAGS + 2,
>> + err = virtqueue_add_buf(rq->vq, rq->sg, 0, MAX_SKB_FRAGS + 2,
>> first, gfp);
>> if (err< 0)
>> - give_pages(vi, first);
>> + give_pages(rq, first);
>>
>> return err;
>> }
>>
>> -static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp)
>> +static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
>> {
>> struct page *page;
>> int err;
>>
>> - page = get_a_page(vi, gfp);
>> + page = get_a_page(rq, gfp);
>> if (!page)
>> return -ENOMEM;
>>
>> - sg_init_one(vi->rx_sg, page_address(page), PAGE_SIZE);
>> + sg_init_one(rq->sg, page_address(page), PAGE_SIZE);
>>
>> - err = virtqueue_add_buf(vi->rvq, vi->rx_sg, 0, 1, page, gfp);
>> + err = virtqueue_add_buf(rq->vq, rq->sg, 0, 1, page, gfp);
>> if (err< 0)
>> - give_pages(vi, page);
>> + give_pages(rq, page);
>>
>> return err;
>> }
>> @@ -458,97 +506,104 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp)
>> * before we're receiving packets, or from refill_work which is
>> * careful to disable receiving (using napi_disable).
>> */
>> -static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
>> +static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp)
>> {
>> + struct virtnet_info *vi = rq->vi;
>> int err;
>> bool oom;
>>
>> do {
>> if (vi->mergeable_rx_bufs)
>> - err = add_recvbuf_mergeable(vi, gfp);
>> + err = add_recvbuf_mergeable(rq, gfp);
>> else if (vi->big_packets)
>> - err = add_recvbuf_big(vi, gfp);
>> + err = add_recvbuf_big(rq, gfp);
>> else
>> - err = add_recvbuf_small(vi, gfp);
>> + err = add_recvbuf_small(rq, gfp);
>>
>> oom = err == -ENOMEM;
>> if (err< 0)
>> break;
>> - ++vi->num;
>> + ++rq->num;
>> } while (err> 0);
>> - if (unlikely(vi->num> vi->max))
>> - vi->max = vi->num;
>> - virtqueue_kick(vi->rvq);
>> + if (unlikely(rq->num> rq->max))
>> + rq->max = rq->num;
>> + virtqueue_kick(rq->vq);
>> return !oom;
>> }
>>
>> -static void skb_recv_done(struct virtqueue *rvq)
>> +static void skb_recv_done(struct virtqueue *vq)
>> {
>> - struct virtnet_info *vi = rvq->vdev->priv;
>> + struct virtnet_info *vi = vq->vdev->priv;
>> + struct napi_struct *napi =&vi->rq[rxq_get_qnum(vi, vq)]->napi;
>> +
>> /* Schedule NAPI, Suppress further interrupts if successful. */
>> - if (napi_schedule_prep(&vi->napi)) {
>> - virtqueue_disable_cb(rvq);
>> - __napi_schedule(&vi->napi);
>> + if (napi_schedule_prep(napi)) {
>> + virtqueue_disable_cb(vq);
>> + __napi_schedule(napi);
>> }
>> }
>>
>> -static void virtnet_napi_enable(struct virtnet_info *vi)
>> +static void virtnet_napi_enable(struct receive_queue *rq)
>> {
>> - napi_enable(&vi->napi);
>> + napi_enable(&rq->napi);
>>
>> /* If all buffers were filled by other side before we napi_enabled, we
>> * won't get another interrupt, so process any outstanding packets
>> * now. virtnet_poll wants re-enable the queue, so we disable here.
>> * We synchronize against interrupts via NAPI_STATE_SCHED */
>> - if (napi_schedule_prep(&vi->napi)) {
>> - virtqueue_disable_cb(vi->rvq);
>> + if (napi_schedule_prep(&rq->napi)) {
>> + virtqueue_disable_cb(rq->vq);
>> local_bh_disable();
>> - __napi_schedule(&vi->napi);
>> + __napi_schedule(&rq->napi);
>> local_bh_enable();
>> }
>> }
>>
>> static void refill_work(struct work_struct *work)
>> {
>> - struct virtnet_info *vi;
>> + struct napi_struct *napi;
>> + struct receive_queue *rq;
>> bool still_empty;
>>
>> - vi = container_of(work, struct virtnet_info, refill.work);
>> - napi_disable(&vi->napi);
>> - still_empty = !try_fill_recv(vi, GFP_KERNEL);
>> - virtnet_napi_enable(vi);
>> + rq = container_of(work, struct receive_queue, refill.work);
>> + napi =&rq->napi;
>> +
>> + napi_disable(napi);
>> + still_empty = !try_fill_recv(rq, GFP_KERNEL);
>> + virtnet_napi_enable(rq);
>>
>> /* In theory, this can happen: if we don't get any buffers in
>> * we will *never* try to fill again. */
>> if (still_empty)
>> - queue_delayed_work(system_nrt_wq,&vi->refill, HZ/2);
>> + queue_delayed_work(system_nrt_wq,&rq->refill, HZ/2);
>> }
>>
>> static int virtnet_poll(struct napi_struct *napi, int budget)
>> {
>> - struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
>> + struct receive_queue *rq = container_of(napi, struct receive_queue,
>> + napi);
>> void *buf;
>> unsigned int len, received = 0;
>>
>> again:
>> while (received< budget&&
>> - (buf = virtqueue_get_buf(vi->rvq,&len)) != NULL) {
>> - receive_buf(vi->dev, buf, len);
>> - --vi->num;
>> + (buf = virtqueue_get_buf(rq->vq,&len)) != NULL) {
>> + receive_buf(rq, buf, len);
>> + --rq->num;
>> received++;
>> }
>>
>> - if (vi->num< vi->max / 2) {
>> - if (!try_fill_recv(vi, GFP_ATOMIC))
>> - queue_delayed_work(system_nrt_wq,&vi->refill, 0);
>> + if (rq->num< rq->max / 2) {
>> + if (!try_fill_recv(rq, GFP_ATOMIC))
>> + queue_delayed_work(system_nrt_wq,&rq->refill, 0);
>> }
>>
>> /* Out of packets? */
>> if (received< budget) {
>> napi_complete(napi);
>> - if (unlikely(!virtqueue_enable_cb(vi->rvq))&&
>> + if (unlikely(!virtqueue_enable_cb(rq->vq))&&
>> napi_schedule_prep(napi)) {
>> - virtqueue_disable_cb(vi->rvq);
>> + virtqueue_disable_cb(rq->vq);
>> __napi_schedule(napi);
>> goto again;
>> }
>> @@ -557,13 +612,14 @@ again:
>> return received;
>> }
>>
>> -static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
>> +static unsigned int free_old_xmit_skbs(struct virtnet_info *vi,
>> + struct virtqueue *vq)
>> {
>> struct sk_buff *skb;
>> unsigned int len, tot_sgs = 0;
>> struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>>
>> - while ((skb = virtqueue_get_buf(vi->svq,&len)) != NULL) {
>> + while ((skb = virtqueue_get_buf(vq,&len)) != NULL) {
>> pr_debug("Sent skb %p\n", skb);
>>
>> u64_stats_update_begin(&stats->tx_syncp);
>> @@ -577,7 +633,8 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
>> return tot_sgs;
>> }
>>
>> -static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
>> +static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb,
>> + struct virtqueue *vq, struct scatterlist *sg)
>> {
>> struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
>> const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
>> @@ -615,44 +672,47 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
>>
>> /* Encode metadata header at front. */
>> if (vi->mergeable_rx_bufs)
>> - sg_set_buf(vi->tx_sg,&hdr->mhdr, sizeof hdr->mhdr);
>> + sg_set_buf(sg,&hdr->mhdr, sizeof hdr->mhdr);
>> else
>> - sg_set_buf(vi->tx_sg,&hdr->hdr, sizeof hdr->hdr);
>> + sg_set_buf(sg,&hdr->hdr, sizeof hdr->hdr);
>>
>> - hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
>> - return virtqueue_add_buf(vi->svq, vi->tx_sg, hdr->num_sg,
>> + hdr->num_sg = skb_to_sgvec(skb, sg + 1, 0, skb->len) + 1;
>> + return virtqueue_add_buf(vq, sg, hdr->num_sg,
>> 0, skb, GFP_ATOMIC);
>> }
>>
>> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> + int qnum = skb_get_queue_mapping(skb);
>> + struct virtqueue *vq = vi->sq[qnum]->vq;
>> int capacity;
>>
>> /* Free up any pending old buffers before queueing new ones. */
>> - free_old_xmit_skbs(vi);
>> + free_old_xmit_skbs(vi, vq);
>>
>> /* Try to transmit */
>> - capacity = xmit_skb(vi, skb);
>> + capacity = xmit_skb(vi, skb, vq, vi->sq[qnum]->sg);
>>
>> /* This can happen with OOM and indirect buffers. */
>> if (unlikely(capacity< 0)) {
>> if (likely(capacity == -ENOMEM)) {
>> if (net_ratelimit())
>> dev_warn(&dev->dev,
>> - "TX queue failure: out of memory\n");
>> + "TXQ (%d) failure: out of memory\n",
>> + qnum);
>> } else {
>> dev->stats.tx_fifo_errors++;
>> if (net_ratelimit())
>> dev_warn(&dev->dev,
>> - "Unexpected TX queue failure: %d\n",
>> - capacity);
>> + "Unexpected TXQ (%d) failure: %d\n",
>> + qnum, capacity);
>> }
>> dev->stats.tx_dropped++;
>> kfree_skb(skb);
>> return NETDEV_TX_OK;
>> }
>> - virtqueue_kick(vi->svq);
>> + virtqueue_kick(vq);
>>
>> /* Don't wait up for transmitted skbs to be freed. */
>> skb_orphan(skb);
>> @@ -661,13 +721,13 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>> /* Apparently nice girls don't return TX_BUSY; stop the queue
>> * before it gets out of hand. Naturally, this wastes entries. */
>> if (capacity< 2+MAX_SKB_FRAGS) {
>> - netif_stop_queue(dev);
>> - if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
>> + netif_stop_subqueue(dev, qnum);
>> + if (unlikely(!virtqueue_enable_cb_delayed(vq))) {
>> /* More just got used, free them then recheck. */
>> - capacity += free_old_xmit_skbs(vi);
>> + capacity += free_old_xmit_skbs(vi, vq);
>> if (capacity>= 2+MAX_SKB_FRAGS) {
>> - netif_start_queue(dev);
>> - virtqueue_disable_cb(vi->svq);
>> + netif_start_subqueue(dev, qnum);
>> + virtqueue_disable_cb(vq);
>> }
>> }
>> }
>> @@ -700,7 +760,8 @@ static struct rtnl_link_stats64 *virtnet_stats(struct net_device *dev,
>> unsigned int start;
>>
>> for_each_possible_cpu(cpu) {
>> - struct virtnet_stats *stats = per_cpu_ptr(vi->stats, cpu);
>> + struct virtnet_stats __percpu *stats
>> + = per_cpu_ptr(vi->stats, cpu);
>> u64 tpackets, tbytes, rpackets, rbytes;
>>
>> do {
>> @@ -734,20 +795,26 @@ static struct rtnl_link_stats64 *virtnet_stats(struct net_device *dev,
>> static void virtnet_netpoll(struct net_device *dev)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> + int i;
>>
>> - napi_schedule(&vi->napi);
>> + for (i = 0; i< vi->num_queue_pairs; i++)
>> + napi_schedule(&vi->rq[i]->napi);
>> }
>> #endif
>>
>> static int virtnet_open(struct net_device *dev)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> + int i;
>>
>> - /* Make sure we have some buffers: if oom use wq. */
>> - if (!try_fill_recv(vi, GFP_KERNEL))
>> - queue_delayed_work(system_nrt_wq,&vi->refill, 0);
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + /* Make sure we have some buffers: if oom use wq. */
>> + if (!try_fill_recv(vi->rq[i], GFP_KERNEL))
>> + queue_delayed_work(system_nrt_wq,
>> + &vi->rq[i]->refill, 0);
>> + virtnet_napi_enable(vi->rq[i]);
>> + }
>>
>> - virtnet_napi_enable(vi);
>> return 0;
>> }
>>
>> @@ -809,10 +876,13 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>> static int virtnet_close(struct net_device *dev)
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>> + int i;
>>
>> /* Make sure refill_work doesn't re-enable napi! */
>> - cancel_delayed_work_sync(&vi->refill);
>> - napi_disable(&vi->napi);
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + cancel_delayed_work_sync(&vi->rq[i]->refill);
>> + napi_disable(&vi->rq[i]->napi);
>> + }
>>
>> return 0;
>> }
>> @@ -924,11 +994,10 @@ static void virtnet_get_ringparam(struct net_device *dev,
>> {
>> struct virtnet_info *vi = netdev_priv(dev);
>>
>> - ring->rx_max_pending = virtqueue_get_vring_size(vi->rvq);
>> - ring->tx_max_pending = virtqueue_get_vring_size(vi->svq);
>> + ring->rx_max_pending = virtqueue_get_vring_size(vi->rq[0]->vq);
>> + ring->tx_max_pending = virtqueue_get_vring_size(vi->sq[0]->vq);
>> ring->rx_pending = ring->rx_max_pending;
>> ring->tx_pending = ring->tx_max_pending;
>> -
>> }
>>
>>
>> @@ -961,6 +1030,19 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
>> return 0;
>> }
>>
>> +/* To avoid contending a lock hold by a vcpu who would exit to host, select the
>> + * txq based on the processor id.
>> + */
>> +static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
>> +{
>> + int txq = skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) :
>> + smp_processor_id();
>> +
>> + while (unlikely(txq>= dev->real_num_tx_queues))
>> + txq -= dev->real_num_tx_queues;
>> + return txq;
>> +}
>> +
>> static const struct net_device_ops virtnet_netdev = {
>> .ndo_open = virtnet_open,
>> .ndo_stop = virtnet_close,
>> @@ -972,6 +1054,7 @@ static const struct net_device_ops virtnet_netdev = {
>> .ndo_get_stats64 = virtnet_stats,
>> .ndo_vlan_rx_add_vid = virtnet_vlan_rx_add_vid,
>> .ndo_vlan_rx_kill_vid = virtnet_vlan_rx_kill_vid,
>> + .ndo_select_queue = virtnet_select_queue,
>> #ifdef CONFIG_NET_POLL_CONTROLLER
>> .ndo_poll_controller = virtnet_netpoll,
>> #endif
>> @@ -1007,10 +1090,10 @@ static void virtnet_config_changed_work(struct work_struct *work)
>>
>> if (vi->status& VIRTIO_NET_S_LINK_UP) {
>> netif_carrier_on(vi->dev);
>> - netif_wake_queue(vi->dev);
>> + netif_tx_wake_all_queues(vi->dev);
>> } else {
>> netif_carrier_off(vi->dev);
>> - netif_stop_queue(vi->dev);
>> + netif_tx_stop_all_queues(vi->dev);
>> }
>> done:
>> mutex_unlock(&vi->config_lock);
>> @@ -1023,41 +1106,217 @@ static void virtnet_config_changed(struct virtio_device *vdev)
>> queue_work(system_nrt_wq,&vi->config_work);
>> }
>>
>> -static int init_vqs(struct virtnet_info *vi)
>> +static void free_receive_bufs(struct virtnet_info *vi)
>> +{
>> + int i;
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + while (vi->rq[i]->pages)
>> + __free_pages(get_a_page(vi->rq[i], GFP_KERNEL), 0);
>> + }
>> +}
>> +
>> +/* Free memory allocated for send and receive queues */
>> +static void virtnet_free_queues(struct virtnet_info *vi)
>> {
>> - struct virtqueue *vqs[3];
>> - vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
>> - const char *names[] = { "input", "output", "control" };
>> - int nvqs, err;
>> + int i;
>>
>> - /* We expect two virtqueues, receive then send,
>> - * and optionally control. */
>> - nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + kfree(vi->rq[i]);
>> + vi->rq[i] = NULL;
>> + kfree(vi->sq[i]);
>> + vi->sq[i] = NULL;
>> + }
>> +}
>>
>> - err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names);
>> - if (err)
>> - return err;
>> +static void free_unused_bufs(struct virtnet_info *vi)
>> +{
>> + void *buf;
>> + int i;
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + struct virtqueue *vq = vi->sq[i]->vq;
>> +
>> + while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
>> + dev_kfree_skb(buf);
>> + }
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + struct virtqueue *vq = vi->rq[i]->vq;
>> +
>> + while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
>> + if (vi->mergeable_rx_bufs || vi->big_packets)
>> + give_pages(vi->rq[i], buf);
>> + else
>> + dev_kfree_skb(buf);
>> + --vi->rq[i]->num;
>> + }
>> + BUG_ON(vi->rq[i]->num != 0);
>> + }
>> +}
>> +
>> +static void virtnet_set_affinity(struct virtnet_info *vi, bool set)
>> +{
>> + int i;
>> +
>> + if (vi->num_queue_pairs == 1)
>> + return;
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + int cpu = set ? i : -1;
>> + virtqueue_set_affinity(vi->rq[i]->vq, cpu);
>> + virtqueue_set_affinity(vi->sq[i]->vq, cpu);
>> + }
>> + return;
>> +}
>> +
>> +static void virtnet_del_vqs(struct virtnet_info *vi)
>> +{
>> + struct virtio_device *vdev = vi->vdev;
>> +
>> + virtnet_set_affinity(vi, false);
>> +
>> + vdev->config->del_vqs(vdev);
>> +
>> + virtnet_free_queues(vi);
>> +}
>> +
>> +static int virtnet_find_vqs(struct virtnet_info *vi)
>> +{
>> + vq_callback_t **callbacks;
>> + struct virtqueue **vqs;
>> + int ret = -ENOMEM;
>> + int i, total_vqs;
>> + char **names;
>>
>> - vi->rvq = vqs[0];
>> - vi->svq = vqs[1];
>> + /*
>> + * We expect 1 RX virtqueue followed by 1 TX virtqueue, followd by
>> + * possible control virtqueue and followed by the same
>> + * 'vi->num_queue_pairs-1' more times
>> + */
>> + total_vqs = vi->num_queue_pairs * 2 +
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ);
>> +
>> + /* Allocate space for find_vqs parameters */
>> + vqs = kmalloc(total_vqs * sizeof(*vqs), GFP_KERNEL);
>> + callbacks = kmalloc(total_vqs * sizeof(*callbacks), GFP_KERNEL);
>> + names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
> so this needs to be kzalloc otherwise on an error cleanup will
> get uninitialized data and crash?
Yes, will change it to use kzalloc.
>
>> + if (!vqs || !callbacks || !names)
>> + goto err;
>> +
>> + /* Parameters for control virtqueue, if any */
>> + if (vi->has_cvq) {
>> + callbacks[2] = NULL;
>> + names[2] = "control";
>> + }
>> +
>> + /* Allocate/initialize parameters for send/receive virtqueues */
>> + for (i = 0; i< vi->num_queue_pairs * 2; i += 2) {
>> + int j = (i == 0 ? i : i + vi->has_cvq);
>> + callbacks[j] = skb_recv_done;
>> + callbacks[j + 1] = skb_xmit_done;
>> + names[j] = kasprintf(GFP_KERNEL, "input.%d", i / 2);
>> + names[j + 1] = kasprintf(GFP_KERNEL, "output.%d", i / 2);
> This needs wrappers. E.g. virtnet_rx_vq(int queue_pair), virtnet_tx_vq(int queue_pair);
> Then you would just scan 0 to num_queue_pairs, and i is queue pair
> number.
Ok.
>> + }
>>
>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
>> + ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
>> + (const char **)names);
>> + if (ret)
>> + goto err;
>> +
>> + if (vi->has_cvq)
>> vi->cvq = vqs[2];
>>
>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
>> - vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
>> + for (i = 0; i< vi->num_queue_pairs * 2; i += 2) {
>> + int j = i == 0 ? i : i + vi->has_cvq;
>> + vi->rq[i / 2]->vq = vqs[j];
>> + vi->sq[i / 2]->vq = vqs[j + 1];
> Same here.
Consider the code is really simple, seem no need to use helpers.
>
>> }
>> - return 0;
>> +
>> +err:
>> + if (ret&& names)
> If we are here ret != 0. For names, just add another label, don't
> complicate cleanup.
Ok.
>> + for (i = 0; i< vi->num_queue_pairs * 2; i++)
>> + kfree(names[i]);
>> +
>> + kfree(names);
>> + kfree(callbacks);
>> + kfree(vqs);
>> +
>> + return ret;
>> +}
>> +
>> +static int virtnet_alloc_queues(struct virtnet_info *vi)
>> +{
>> + int ret = -ENOMEM;
>> + int i;
>> +
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + vi->rq[i] = kzalloc(sizeof(*vi->rq[i]), GFP_KERNEL);
>> + vi->sq[i] = kzalloc(sizeof(*vi->sq[i]), GFP_KERNEL);
>> + if (!vi->rq[i] || !vi->sq[i])
>> + goto err;
>> + }
>> +
>> + ret = 0;
>> +
>> + /* setup initial receive and send queue parameters */
>> + for (i = 0; i< vi->num_queue_pairs; i++) {
>> + vi->rq[i]->vi = vi;
>> + vi->rq[i]->pages = NULL;
>> + INIT_DELAYED_WORK(&vi->rq[i]->refill, refill_work);
>> + netif_napi_add(vi->dev,&vi->rq[i]->napi, virtnet_poll,
>> + napi_weight);
>> +
>> + sg_init_table(vi->rq[i]->sg, ARRAY_SIZE(vi->rq[i]->sg));
>> + sg_init_table(vi->sq[i]->sg, ARRAY_SIZE(vi->sq[i]->sg));
>> + }
>> +
> Add return 0 here, then ret = 0 will not be needed
> above and if (ret) below.
>
Ok.
>> +err:
>> + if (ret)
>> + virtnet_free_queues(vi);
>> +
>> + return ret;
>> +}
>> +
>> +static int virtnet_setup_vqs(struct virtnet_info *vi)
>> +{
>> + int ret;
>> +
>> + /* Allocate send& receive queues */
>> + ret = virtnet_alloc_queues(vi);
>> + if (!ret) {
>> + ret = virtnet_find_vqs(vi);
>> + if (ret)
>> + virtnet_free_queues(vi);
>> + else
>> + virtnet_set_affinity(vi, true);
>> + }
>> +
>> + return ret;
> Add some labels for error handling, this if nesting is messy.
Ok.
>> }
>>
>> static int virtnet_probe(struct virtio_device *vdev)
>> {
>> - int err;
>> + int i, err;
>> struct net_device *dev;
>> struct virtnet_info *vi;
>> + u16 num_queues, num_queue_pairs;
>> +
>> + /* Find if host supports multiqueue virtio_net device */
>> + err = virtio_config_val(vdev, VIRTIO_NET_F_MULTIQUEUE,
>> + offsetof(struct virtio_net_config,
>> + num_queues),&num_queues);
>> +
>> + /* We need atleast 2 queue's */
> typo
Will fix this, thanks.
>> + if (err || num_queues< 2)
>> + num_queues = 2;
>> + if (num_queues> MAX_QUEUES * 2)
>> + num_queues = MAX_QUEUES;
>> +
>> + num_queue_pairs = num_queues / 2;
>>
>> /* Allocate ourselves a network device with room for our info */
>> - dev = alloc_etherdev(sizeof(struct virtnet_info));
>> + dev = alloc_etherdev_mq(sizeof(struct virtnet_info), num_queue_pairs);
>> if (!dev)
>> return -ENOMEM;
>>
>> @@ -1103,22 +1362,18 @@ static int virtnet_probe(struct virtio_device *vdev)
>>
>> /* Set up our device-specific information */
>> vi = netdev_priv(dev);
>> - netif_napi_add(dev,&vi->napi, virtnet_poll, napi_weight);
>> vi->dev = dev;
>> vi->vdev = vdev;
>> vdev->priv = vi;
>> - vi->pages = NULL;
>> vi->stats = alloc_percpu(struct virtnet_stats);
>> err = -ENOMEM;
>> if (vi->stats == NULL)
>> - goto free;
>> + goto free_netdev;
>>
>> - INIT_DELAYED_WORK(&vi->refill, refill_work);
>> mutex_init(&vi->config_lock);
>> vi->config_enable = true;
>> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
>> - sg_init_table(vi->rx_sg, ARRAY_SIZE(vi->rx_sg));
>> - sg_init_table(vi->tx_sg, ARRAY_SIZE(vi->tx_sg));
>> + vi->num_queue_pairs = num_queue_pairs;
>>
>> /* If we can receive ANY GSO packets, we must allocate large ones. */
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> @@ -1129,9 +1384,17 @@ static int virtnet_probe(struct virtio_device *vdev)
>> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
>> vi->mergeable_rx_bufs = true;
>>
>> - err = init_vqs(vi);
>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>> + vi->has_cvq = true;
>> +
> How about we disable multiqueue if there's no cvq?
> Will make logic a bit simpler, won't it?
We can, but as you said, just let the logic simpler a bit.
>> + /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
>> + err = virtnet_setup_vqs(vi);
>> if (err)
>> - goto free_stats;
>> + goto free_netdev;
>> +
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)&&
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
>> + dev->features |= NETIF_F_HW_VLAN_FILTER;
>>
>> err = register_netdev(dev);
>> if (err) {
>> @@ -1140,12 +1403,15 @@ static int virtnet_probe(struct virtio_device *vdev)
>> }
>>
>> /* Last of all, set up some receive buffers. */
>> - try_fill_recv(vi, GFP_KERNEL);
>> -
>> - /* If we didn't even get one input buffer, we're useless. */
>> - if (vi->num == 0) {
>> - err = -ENOMEM;
>> - goto unregister;
>> + for (i = 0; i< num_queue_pairs; i++) {
>> + try_fill_recv(vi->rq[i], GFP_KERNEL);
>> +
>> + /* If we didn't even get one input buffer, we're useless. */
>> + if (vi->rq[i]->num == 0) {
>> + free_unused_bufs(vi);
>> + err = -ENOMEM;
>> + goto free_recv_bufs;
>> + }
>> }
>>
>> /* Assume link up if device can't report link status,
>> @@ -1158,42 +1424,25 @@ static int virtnet_probe(struct virtio_device *vdev)
>> netif_carrier_on(dev);
>> }
>>
>> - pr_debug("virtnet: registered device %s\n", dev->name);
>> + pr_debug("virtnet: registered device %s with %d RX and TX vq's\n",
>> + dev->name, num_queue_pairs);
>> +
>> return 0;
>>
>> -unregister:
>> +free_recv_bufs:
>> + free_receive_bufs(vi);
>> unregister_netdev(dev);
>> +
>> free_vqs:
>> - vdev->config->del_vqs(vdev);
>> -free_stats:
>> - free_percpu(vi->stats);
>> -free:
>> + for (i = 0; i< num_queue_pairs; i++)
>> + cancel_delayed_work_sync(&vi->rq[i]->refill);
>> + virtnet_del_vqs(vi);
>> +
>> +free_netdev:
>> free_netdev(dev);
>> return err;
>> }
>>
>> -static void free_unused_bufs(struct virtnet_info *vi)
>> -{
>> - void *buf;
>> - while (1) {
>> - buf = virtqueue_detach_unused_buf(vi->svq);
>> - if (!buf)
>> - break;
>> - dev_kfree_skb(buf);
>> - }
>> - while (1) {
>> - buf = virtqueue_detach_unused_buf(vi->rvq);
>> - if (!buf)
>> - break;
>> - if (vi->mergeable_rx_bufs || vi->big_packets)
>> - give_pages(vi, buf);
>> - else
>> - dev_kfree_skb(buf);
>> - --vi->num;
>> - }
>> - BUG_ON(vi->num != 0);
>> -}
>> -
>> static void remove_vq_common(struct virtnet_info *vi)
>> {
>> vi->vdev->config->reset(vi->vdev);
>> @@ -1201,10 +1450,9 @@ static void remove_vq_common(struct virtnet_info *vi)
>> /* Free unused buffers in both send and recv, if any. */
>> free_unused_bufs(vi);
>>
>> - vi->vdev->config->del_vqs(vi->vdev);
>> + free_receive_bufs(vi);
>>
>> - while (vi->pages)
>> - __free_pages(get_a_page(vi, GFP_KERNEL), 0);
>> + virtnet_del_vqs(vi);
>> }
>>
>> static void __devexit virtnet_remove(struct virtio_device *vdev)
>> @@ -1230,6 +1478,7 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
>> static int virtnet_freeze(struct virtio_device *vdev)
>> {
>> struct virtnet_info *vi = vdev->priv;
>> + int i;
>>
>> /* Prevent config work handler from accessing the device */
>> mutex_lock(&vi->config_lock);
>> @@ -1237,10 +1486,13 @@ static int virtnet_freeze(struct virtio_device *vdev)
>> mutex_unlock(&vi->config_lock);
>>
>> netif_device_detach(vi->dev);
>> - cancel_delayed_work_sync(&vi->refill);
>> + for (i = 0; i< vi->num_queue_pairs; i++)
>> + cancel_delayed_work_sync(&vi->rq[i]->refill);
>>
>> if (netif_running(vi->dev))
>> - napi_disable(&vi->napi);
>> + for (i = 0; i< vi->num_queue_pairs; i++)
>> + napi_disable(&vi->rq[i]->napi);
>> +
>>
>> remove_vq_common(vi);
>>
>> @@ -1252,19 +1504,22 @@ static int virtnet_freeze(struct virtio_device *vdev)
>> static int virtnet_restore(struct virtio_device *vdev)
>> {
>> struct virtnet_info *vi = vdev->priv;
>> - int err;
>> + int err, i;
>>
>> - err = init_vqs(vi);
>> + err = virtnet_setup_vqs(vi);
>> if (err)
>> return err;
>>
>> if (netif_running(vi->dev))
>> - virtnet_napi_enable(vi);
>> + for (i = 0; i< vi->num_queue_pairs; i++)
>> + virtnet_napi_enable(vi->rq[i]);
>>
>> netif_device_attach(vi->dev);
>>
>> - if (!try_fill_recv(vi, GFP_KERNEL))
>> - queue_delayed_work(system_nrt_wq,&vi->refill, 0);
>> + for (i = 0; i< vi->num_queue_pairs; i++)
>> + if (!try_fill_recv(vi->rq[i], GFP_KERNEL))
>> + queue_delayed_work(system_nrt_wq,
>> + &vi->rq[i]->refill, 0);
>>
>> mutex_lock(&vi->config_lock);
>> vi->config_enable = true;
>> @@ -1287,7 +1542,7 @@ static unsigned int features[] = {
>> VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
>> VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
>> - VIRTIO_NET_F_GUEST_ANNOUNCE,
>> + VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MULTIQUEUE,
>> };
>>
>> static struct virtio_driver virtio_net_driver = {
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 1bc7e30..60f09ff 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -61,6 +61,8 @@ struct virtio_net_config {
>> __u8 mac[6];
>> /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
>> __u16 status;
>> + /* Total number of RX/TX queues */
>> + __u16 num_queues;
>> } __attribute__((packed));
>>
>> /* This is the first element of the scatter-gather list. If you don't
>> --
>> 1.7.1
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Jason Wang @ 2012-07-23 5:54 UTC (permalink / raw)
To: Sasha Levin
Cc: krkumar2, habanero, mashirle, kvm, Michael S. Tsirkin, netdev,
linux-kernel, virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <500A9A72.20507@gmail.com>
On 07/21/2012 08:02 PM, Sasha Levin wrote:
> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>> - err = init_vqs(vi);
>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>> + vi->has_cvq = true;
>>>> +
>> How about we disable multiqueue if there's no cvq?
>> Will make logic a bit simpler, won't it?
> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
>
Yes, it does not depends on cvq. Cvq were just used to negotiate the
number of queues a guest wishes to use which is really useful (at least
for now). Since multiqueue can not out-perform for single queue in every
kinds of workloads or benchmark, so we want to let guest driver use
single queue by default even when multiqueue were enabled by management
software and let use to enalbe it through ethtool. So user could not
feel regression when it switch to use a multiqueue capable driver and
backend.
So the only difference is the user experiences.
> --
> 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/
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Sasha Levin @ 2012-07-23 9:28 UTC (permalink / raw)
To: Jason Wang
Cc: krkumar2, habanero, mashirle, kvm, Michael S. Tsirkin, netdev,
linux-kernel, virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <500CE72B.2040101@redhat.com>
On 07/23/2012 07:54 AM, Jason Wang wrote:
> On 07/21/2012 08:02 PM, Sasha Levin wrote:
>> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>>> - err = init_vqs(vi);
>>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>>> + vi->has_cvq = true;
>>>>> +
>>> How about we disable multiqueue if there's no cvq?
>>> Will make logic a bit simpler, won't it?
>> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
>>
>
> Yes, it does not depends on cvq. Cvq were just used to negotiate the number of queues a guest wishes to use which is really useful (at least for now). Since multiqueue can not out-perform for single queue in every kinds of workloads or benchmark, so we want to let guest driver use single queue by default even when multiqueue were enabled by management software and let use to enalbe it through ethtool. So user could not feel regression when it switch to use a multiqueue capable driver and backend.
Why would you limit it to a single vq if the user has specified a different number of vqs (>1) in the virtio-net device config?
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-07-23 18:19 UTC (permalink / raw)
To: Minchan Kim
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Andrew Morton,
Rafael Aquini
In-Reply-To: <20120723023332.GA6832@bbox>
On Mon, Jul 23, 2012 at 11:33:32AM +0900, Minchan Kim wrote:
> Hi Rafael,
>
> On Fri, Jul 20, 2012 at 04:48:59PM -0300, Rafael Aquini wrote:
> > Howdy Minchan,
> >
> > Once again, thanks for raising such valuable feedback over here.
> >
> > On Wed, Jul 18, 2012 at 02:48:24PM +0900, Minchan Kim wrote:
> > > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > > +static bool isolate_balloon_page(struct page *page)
> > > > +{
> > > > + if (WARN_ON(!is_balloon_page(page)))
> > > > + return false;
> > >
> > > I am not sure we need this because you alreay check it before calling
> > > isolate_balloon_page. If you really need it, it would be better to
> > > add likely in isolate_balloon_page, too.
> > >
> >
> > This check point was set in place because isolate_balloon_page() was a publicly
> > visible function and while our current usage looks correct it would not hurt to
> > have something like that done -- think of it as an insurance policy, in case
> > someone else, in the future, attempts to use it on any other place outside this
> > specifc context.
> > Despite not seeing it as a dealbreaker for the patch as is, I do agree, however,
> > this snippet can _potentially_ be removed from isolate_balloon_page(), since
> > this function has become static to compaction.c.
>
> Yes. It's not static.
>
> >
> >
> > > > +
> > > > + if (likely(get_page_unless_zero(page))) {
> > > > + /*
> > > > + * We can race against move_to_new_page() & __unmap_and_move().
> > > > + * If we stumble across a locked balloon page and succeed on
> > > > + * isolating it, the result tends to be disastrous.
> > > > + */
> > > > + if (likely(trylock_page(page))) {
> > >
> > > Hmm, I can't understand your comment.
> > > What does this lock protect? Could you elaborate it with code sequence?
> > >
> >
> > As we are coping with a corner case -- balloon pages are not on LRU lists --
> > compaction concurrency can lead to a disastrous race which results in
> > isolate_balloon_page() re-isolating already isolated balloon pages, or isolating
> > a 'newpage' recently promoted to 'balloon page', while these pages are still
> > under migration. The only way we have to prevent that from happening is
> > attempting to grab the page lock, as pages under migration are already locked.
> > I had that comment rephrased to what follows (please, tell me how it sounds to
> > you now)
> > if (likely(get_page_unless_zero(page))) {
> > /*
> > - * We can race against move_to_new_page() & __unmap_and_move().
> > - * If we stumble across a locked balloon page and succeed on
> > - * isolating it, the result tends to be disastrous.
> > + * As balloon pages are not isolated from LRU lists, several
> > + * concurrent compaction threads will potentially race against
> > + * page migration at move_to_new_page() & __unmap_and_move().
> > + * In order to avoid having an already isolated balloon page
> > + * being (wrongly) re-isolated while it is under migration,
> > + * lets be sure we have the page lock before proceeding with
> > + * the balloon page isolation steps.
>
> Looks good to me.
>
> > */
> > if (likely(trylock_page(page))) {
> > /*
> >
> >
> > > > +/* putback_lru_page() counterpart for a ballooned page */
> > > > +bool putback_balloon_page(struct page *page)
> > > > +{
> > > > + if (WARN_ON(!is_balloon_page(page)))
> > > > + return false;
> > >
> > > You already check WARN_ON in putback_lru_pages so we don't need it in here.
> > > And you can add likely in here, too.
> > >
> >
> > This check point is in place by the same reason why it is for
> > isolate_balloon_page(). However, I don't think we should drop it here because
> > putback_balloon_page() is still a publicly visible symbol. Besides, the check
> > that is performed at putback_lru_pages() level has a different meaning, which is
> > to warn us when we fail on re-inserting an isolated (but not migrated) page back
> > to the balloon page list, thus it does not superceeds nor it's superceeded by
> > this checkpoint here.
>
> I'm not against you strongly but IMHO, putback_balloon_page is never generic
> function which is likely to be used by many others so I hope not overdesign.
>
> >
> >
> > > > + } else if (is_balloon_page(page)) {
> > >
> > > unlikely?
> >
> > This can be done, for sure. Also, it reminds me that I had a
> > 'likely(PageLRU(page))' set in place for this vary same patch, on v2 submission.
> > Do you recollect you've asked me to get rid of it?. Wouldn't it be better having
> > that suggestion of yours reverted, since PageLRU(page) is the likelihood case
> > here anyway? What about this?
> >
> > " if (likely(PageLRU(page))) {
> > ...
> > } else if (unlikely(is_balloon_page(page))) {
> > ...
> > } else
> > continue;
> > "
>
> I don't like that.
> PageLRU isn't likelihood case in this.
>
> >
> > >
> > > > @@ -78,7 +78,10 @@ void putback_lru_pages(struct list_head *l)
> > > > list_del(&page->lru);
> > > > dec_zone_page_state(page, NR_ISOLATED_ANON +
> > > > page_is_file_cache(page));
> > > > - putback_lru_page(page);
> > > > + if (unlikely(is_balloon_page(page)))
> > > > + WARN_ON(!putback_balloon_page(page));
> > > > + else
> > > > + putback_lru_page(page);
> > >
> > > Hmm, I don't like this.
> > > The function name says we putback *lru* pages, but balloon page isn't.
> > > How about this?
> > >
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index aad0a16..b07cd67 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -298,6 +298,8 @@ static bool too_many_isolated(struct zone *zone)
> > > * Apart from cc->migratepages and cc->nr_migratetypes this function
> > > * does not modify any cc's fields, in particular it does not modify
> > > * (or read for that matter) cc->migrate_pfn.
> > > + *
> > > + * For returning page, you should use putback_pages instead of putback_lru_pages
> > > */
> > > unsigned long
> > > isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> > > @@ -827,7 +829,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> > >
> > > /* Release LRU pages not migrated */
> > > if (err) {
> > > - putback_lru_pages(&cc->migratepages);
> > > + putback_pages(&cc->migratepages);
> > > cc->nr_migratepages = 0;
> > > if (err == -ENOMEM) {
> > > ret = COMPACT_PARTIAL;
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 9705e70..a96b840 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -86,6 +86,22 @@ void putback_lru_pages(struct list_head *l)
> > > }
> > > }
> > >
> > > + /* blah blah .... */
> > > +void putback_pages(struct list_head *l)
> > > +{
> > > + struct page *page;
> > > + struct page *page2;
> > > +
> > > + list_for_each_entry_safe(page, page2, l, lru) {
> > > + list_del(&page->lru);
> > > + dec_zone_page_state(page, NR_ISOLATED_ANON +
> > > + page_is_file_cache(page));
> > > + if (unlikely(is_balloon_page(page)))
> > > + WARN_ON(!putback_balloon_page(page));
> > > + else
> > > + putback_lru_page(page);
> > > + }
> > > +}
> > > +
> > > /*
> > > * Restore a potential migration pte to a working pte entry
> > > */
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 32985dd..decb82a 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -5655,7 +5655,7 @@ static int __alloc_contig_migrate_range(unsigned long start, unsigned long end)
> > > 0, false, MIGRATE_SYNC);
> > > }
> > >
> > > - putback_lru_pages(&cc.migratepages);
> > > + putback_pages(&cc.migratepages);
> > > return ret > 0 ? 0 : ret;
> > > }
> > >
> >
> > Despite being a very nice cleanup, this code refactoring has nothing to do with
> > this particular patch purpose. For the sake of this implementation, think about
> > the balloon page list acting as a particular LRU, so although ballooned pages
> > are not enlisted on real LRUs, per se, this doesn't mean we cannot have them
> > dealt as a corner case amongst putback_lru_pages() code for the sake of
> > simplicity and maintainability ease. OTOH, I do agree with your nice suggestion,
> > thus I can submit it as a separate clean-up patch attached to this series (if
> > you don't mind).
>
> No problem. But the concern is that I can't agree this patch implementation.
> Please see below.
>
> >
> >
> > > >
> > > > + if (is_balloon_page(page)) {
> > >
> > > unlikely?
> > >
> >
> > Yeah, why not? Will do it.
> >
> > > > + if (is_balloon_page(newpage)) {
> > >
> > > unlikely?
> > >
> >
> > ditto.
> >
> > > > + /*
> > > > + * A ballooned page has been migrated already. Now, it is the
> > > > + * time to wrap-up counters, handle the old page back to Buddy
> > > > + * and return.
> > > > + */
> > > > + list_del(&page->lru);
> > > > + dec_zone_page_state(page, NR_ISOLATED_ANON +
> > > > + page_is_file_cache(page));
> > > > + put_page(page);
> > > > + __free_page(page);
> > >
> > > Why do you use __free_page instead of put_page?
> > >
> >
> > Do you mean perform an extra put_page() and let putback_lru_page() do the rest?
> > Besides looking odd at code level, what other improvement we can potentially gain here?
>
> I mean just do
> put_page(page); /* blah blah */
> put_page(page); /* It will free the page to buddy */
>
> Yeb it's more fat than __free_page for your case. If so, please comment write out
> why you use __free_page instead of put_page.
>
> /*
> * balloon page should be never compund page and not on the LRU so we
> * call __free_page directly instead of put_page.
> */
>
No, it's ugly and potentially buggy (for this case). Besides, it shows no gain
at all, as ballooned pages are a corner case here and they're not on LRUs. So,
being quite reasonable, how this suggestion of yours could possibly be
acceptable here?
The comment is also not necessary, as there already is a comment on this, properly
placed, which was (apparently) ignored by your eyes.
> >
> >
> > > The feeling I look at your code in detail is that it makes compaction/migration
> > > code rather complicated because compaction/migration assumes source/target would
> > > be LRU pages.
> > >
> > > How often memory ballooning happens? Does it make sense to hook it in generic
> > > functions if it's very rare?
> > >
> > > Couldn't you implement it like huge page?
> > > It doesn't make current code complicated and doesn't affect performance
> > >
> > > In compaction,
> > > #ifdef CONFIG_VIRTIO_BALLOON
> > > static int compact_zone(struct zone *zone, struct compact_control *cc, bool balloon)
> > > {
> > > if (balloon) {
> > > isolate_balloonpages
> > >
> > > migrate_balloon_pages
> > > unmap_and_move_balloon_page
> > >
> > > putback_balloonpages
> > > }
> > > }
> > > #endif
> > >
> > > I'm not sure memory ballooning so it might be dumb question.
> > > Can we trigger it once only when ballooning happens?
> >
> > I strongly believe, this is the simplest and easiest way to get this task
> > accomplished, mostly becasue:
> > i. It does not require any code duplication at all;
> > ii. It requires very few code insertion/surgery to be fully operative;
> > iii. It is embeded on already well established and maintained code;
> > iv. The approach makes easier to other balloon drivers leverage compaction
> > code;
> > v. It shows no significative impact to the entangled code paths;
>
> But it is adding a few hooks to generic functions which all assume they
> are LRU pages. Although it's trivial for performance, we don't need get
> such loss due to very infrequent event.
>
No, it is simply teaching compaction and migration about another possible and
plausible page type case. There is also no loss, the numbers I provided you
support that. The apparent overhead observed on the patched case is, actually,
due to the bigger throughput observed.
In fact, the changes introduced here seem to have (accidentally) made that code
more effective. Checking the numbers, again:
- "compact_pages_moved" average throughput increase:
$ echo "scale=3; (1-(42219/51794))*100" | bc
18.500
- Avg time elapsed (sec) / page moved:
* 3.5.0-rc7 (clean)
$ echo "scale=10; 0.344474338/42219" | bc
.0000081592
* 3.5.0-rc7 (patch)
$ echo "scale=10; 0.404462151/51794" | bc
.0000078090
> More concern to me that it makes code complicated because we assume
> functions related migration/compaction depend on LRU pages.
> Let me say a example.
> suitable_migration_target in isolate_freepages returns true pageblock
> type is MIGRATE_MOVABLE or MIGRATE_CMA which expect pages in that block
> is migratable so they should be moved into another location or reclaimed
> if we have no space for getting big contiguos space.
> All of such code should consider balloon page but balloon page can't be
> reclaimed unlike normal LRU page so it makes design more complex.
>
You're simply ignoring that this series makes ballooned pages movable, so
there's no need for this special treatment you're quoting at
isolate_freepages_*() level. Ballooned pages don't need to be reclaimed, they
just need to be moved away to not cause too much fragmentation.
> Look at memory-hotplug, offline_page calls has_unmovable_pages, scan_lru_pages
> and do_migrate_range which calls isolate_lru_page. They consider only LRU pages
> to migratable ones.
>
A-ha! Now you gave me the real reason why you're struggling against this
proposed implementation. I'll take a look at those bits, to come up with
something suitable, for sure.
> IMHO, better approach is that after we can get complete free pageblocks
> by compaction or reclaim, move balloon pages into that pageblocks and make
> that blocks to unmovable. It can prevent fragmentation and it makes
> current or future code don't need to consider balloon page.
>
> Of course, it needs more code but I think it's valuable.
> What do you think about it?
>
In a glance, I believe this whole dance you're suggesting might just be too much
of an overcomplication, and the best approach would be simply teaching the
hotplug bits about the ballooned corner case just like it's being done to
compaction/migration. However, I'll look at it carefully before making any other
adjustments/propositions over here.
Thanks for your (always) valuable feedback.
Best regards,
Rafael
> --
> Kind regards,
> Minchan Kim
^ permalink raw reply
* [RFC 0/2] virtio: provide a way for host to monitor critical events in the device
From: Sasha Levin @ 2012-07-23 20:32 UTC (permalink / raw)
To: rusty, mst, penberg, asias.hejun
Cc: kvm, wency, linux-kernel, virtualization, avi, anthony,
Sasha Levin
As it was discussed recently, there's currently no way for the guest to notify
the host about panics. Further more, there's no reasonable way to notify the
host of other critical events such as an OOM kill.
This short patch series introduces a new device named virtio-notifier which
does two simple things:
1. Provide a simple interface for the guest to notify the host of critical
events. This is easily expandible to add support for any events we may find
interesting for the host to know about.
2. Provide an "echo" interface for the host to ping the guest. This allows
the host to ping the guest at intervals chosen by the host, and act
accordingly if no response has been received.
Sasha Levin (2):
virtio: Introduce virtio-notifier
kvm tools: support virtio-notifier
drivers/virtio/Kconfig | 11 ++
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_notifier.c | 135 ++++++++++++++++++++
include/linux/virtio_ids.h | 1 +
include/linux/virtio_notifier.h | 15 +++
tools/kvm/Makefile | 1 +
tools/kvm/builtin-run.c | 6 +
tools/kvm/include/kvm/virtio-notifier.h | 9 ++
tools/kvm/include/kvm/virtio-pci-dev.h | 1 +
tools/kvm/virtio/notifier.c | 203 +++++++++++++++++++++++++++++++
10 files changed, 383 insertions(+), 0 deletions(-)
create mode 100644 drivers/virtio/virtio_notifier.c
create mode 100644 include/linux/virtio_notifier.h
create mode 100644 tools/kvm/include/kvm/virtio-notifier.h
create mode 100644 tools/kvm/virtio/notifier.c
--
1.7.8.6
^ permalink raw reply
* [RFC 1/2] virtio: Introduce virtio-notifier
From: Sasha Levin @ 2012-07-23 20:32 UTC (permalink / raw)
To: rusty, mst, penberg, asias.hejun
Cc: kvm, wency, linux-kernel, virtualization, avi, anthony,
Sasha Levin
In-Reply-To: <1343075561-29316-1-git-send-email-levinsasha928@gmail.com>
[TODO: Find a better name]
virtio-notifier is a new driver which provides guests the ability to report
critical events such as a panic or OOM to the host.
The driver also provides an "echo" channel which is primed with a buffer when
the driver is initialized, and allows the host to "ping" the guest to make
sure the guest is still alive and well.
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/virtio/Kconfig | 11 +++
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_notifier.c | 135 ++++++++++++++++++++++++++++++++++++++
include/linux/virtio_ids.h | 1 +
include/linux/virtio_notifier.h | 15 ++++
5 files changed, 163 insertions(+), 0 deletions(-)
create mode 100644 drivers/virtio/virtio_notifier.c
create mode 100644 include/linux/virtio_notifier.h
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 1a61939..1be8f93 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -46,4 +46,15 @@ config VIRTIO_BALLOON
If unsure, say N.
+config VIRTIO_NOTIFIER
+ tristate "Virtio notifier driver (EXPERIMENTAL)"
+ select VIRTIO
+ select VIRTIO_RING
+ ---help---
+ This driver provides support for passing improtant notifications such as
+ notification about guest PANIC or OOM back to the host.
+
+ Also, the driver provides a mechanism to detect lockups in the guest (similar
+ to a watchdog), notifying the host about such lockups.
+
endmenu
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 5a4c63c..7b77d0b 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_VIRTIO_RING) += virtio_ring.o
obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
+obj-$(CONFIG_VIRTIO_NOTIFIER) += virtio_notifier.o
diff --git a/drivers/virtio/virtio_notifier.c b/drivers/virtio/virtio_notifier.c
new file mode 100644
index 0000000..77393c5
--- /dev/null
+++ b/drivers/virtio/virtio_notifier.c
@@ -0,0 +1,135 @@
+/*
+ * Notifier/watchdog driver for virtio
+ * Copyright (C) 2012 Sasha Levin
+ */
+
+#include <linux/err.h>
+#include <linux/scatterlist.h>
+#include <linux/spinlock.h>
+#include <linux/virtio.h>
+#include <linux/virtio_notifier.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/oom.h>
+
+static struct virtqueue *echo, *notif;
+static u32 notif_cnt[VIRTIO_NOTIF_CNT];
+static u64 echo_val;
+
+static void notif_done(struct virtqueue *vq)
+{
+}
+
+static void echo_done(struct virtqueue *vq)
+{
+ struct scatterlist sg;
+
+ sg_init_one(&sg, &echo_val, sizeof(echo_val));
+ if (virtqueue_add_buf(echo, &sg, 1, 0, echo, GFP_KERNEL) < 0)
+ BUG();
+
+ virtqueue_kick(echo);
+}
+
+static void notify_host(int notification)
+{
+ struct scatterlist sg;
+
+ notif_cnt[notification]++;
+
+ sg_init_one(&sg, notif_cnt, sizeof(notif_cnt));
+
+ if (virtqueue_add_buf(notif, &sg, 0, 1, notif, GFP_KERNEL) < 0)
+ BUG();
+
+ virtqueue_kick(notif);
+}
+
+static int virtio_notif_panic(struct notifier_block *this, unsigned long ev, void *ptr)
+{
+ notify_host(VIRTIO_NOTIF_PANIC);
+
+ return NOTIFY_DONE;
+}
+
+static int virtio_notif_oom(struct notifier_block *this, unsigned long ev, void *ptr)
+{
+ notify_host(VIRTIO_NOTIF_OOM);
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block virtio_notif_panic_block = {
+ .notifier_call = virtio_notif_panic,
+};
+
+static struct notifier_block virtio_notif_oom_block = {
+ .notifier_call = virtio_notif_oom,
+};
+
+static int virtnotif_probe(struct virtio_device *vdev)
+{
+ int err;
+ struct virtqueue *vqs[2];
+ vq_callback_t *callbacks[] = { notif_done, echo_done };
+ const char *names[] = { "notif", "echo" };
+ struct scatterlist sg;
+
+ /* We expect two virtqueue. */
+ err = vdev->config->find_vqs(vdev, 2, vqs, callbacks, names);
+ if (err)
+ return err;
+
+ notif = vqs[0];
+ echo = vqs[1];
+
+ /*
+ * Prime this virtqueue with one buffer so the hypervisor can
+ * use it to signal us later.
+ */
+ sg_init_one(&sg, &echo_val, sizeof(echo_val));
+ if (virtqueue_add_buf(echo, &sg, 1, 0, echo, GFP_KERNEL) < 0)
+ BUG();
+
+ virtqueue_kick(echo);
+
+ atomic_notifier_chain_register(&panic_notifier_list, &virtio_notif_panic_block);
+ register_oom_notifier(&virtio_notif_oom_block);
+
+ return 0;
+}
+
+static void __devexit virtnotif_remove(struct virtio_device *vdev)
+{
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);
+}
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_NOTIFIER, VIRTIO_DEV_ANY_ID },
+ { 0 },
+};
+
+static struct virtio_driver virtio_notifier_driver = {
+ .driver.name = KBUILD_MODNAME,
+ .driver.owner = THIS_MODULE,
+ .id_table = id_table,
+ .probe = virtnotif_probe,
+ .remove = __devexit_p(virtnotif_remove),
+};
+
+static int __init init(void)
+{
+ return register_virtio_driver(&virtio_notifier_driver);
+}
+
+static void __exit fini(void)
+{
+ unregister_virtio_driver(&virtio_notifier_driver);
+}
+module_init(init);
+module_exit(fini);
+
+MODULE_DEVICE_TABLE(virtio, id_table);
+MODULE_DESCRIPTION("Virtio notifier driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 7529b85..553edf9 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -34,6 +34,7 @@
#define VIRTIO_ID_CONSOLE 3 /* virtio console */
#define VIRTIO_ID_RNG 4 /* virtio ring */
#define VIRTIO_ID_BALLOON 5 /* virtio balloon */
+#define VIRTIO_ID_NOTIFIER 6 /* virtio notifier */
#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
#define VIRTIO_ID_SCSI 8 /* virtio scsi */
#define VIRTIO_ID_9P 9 /* 9p virtio console */
diff --git a/include/linux/virtio_notifier.h b/include/linux/virtio_notifier.h
new file mode 100644
index 0000000..ad5bcf7
--- /dev/null
+++ b/include/linux/virtio_notifier.h
@@ -0,0 +1,15 @@
+#ifndef _LINUX_VIRTIO_NOTIFIER_H
+#define _LINUX_VIRTIO_NOTIFIER_H
+/* This header is BSD licensed so anyone can use the definitions to implement
+ * compatible drivers/servers. */
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+
+enum virtio_notifications {
+ VIRTIO_NOTIF_PANIC,
+ VIRTIO_NOTIF_OOM,
+
+ VIRTIO_NOTIF_CNT
+};
+
+#endif /* _LINUX_VIRTIO_RNG_H */
--
1.7.8.6
^ permalink raw reply related
* [RFC 2/2] kvm tools: support virtio-notifier
From: Sasha Levin @ 2012-07-23 20:32 UTC (permalink / raw)
To: rusty, mst, penberg, asias.hejun
Cc: kvm, wency, linux-kernel, virtualization, avi, anthony,
Sasha Levin
In-Reply-To: <1343075561-29316-1-git-send-email-levinsasha928@gmail.com>
This patch supports the new virtio-notifier driver.
When the guest experiences a panic or an OOM, it will notify the host and
the host will notify the user of the event.
We also ping echo packets every second to the guest to see if it's still
alive and well, I haven't actually taken care of the case when echos are
gone for some reason since I'm trying to think of something useful to do
with that besides yell on stdout.
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
tools/kvm/Makefile | 1 +
tools/kvm/builtin-run.c | 6 +
tools/kvm/include/kvm/virtio-notifier.h | 9 ++
tools/kvm/include/kvm/virtio-pci-dev.h | 1 +
tools/kvm/virtio/notifier.c | 203 +++++++++++++++++++++++++++++++
5 files changed, 220 insertions(+), 0 deletions(-)
create mode 100644 tools/kvm/include/kvm/virtio-notifier.h
create mode 100644 tools/kvm/virtio/notifier.c
diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index f9e1ec1..6827c1f 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -89,6 +89,7 @@ OBJS += hw/pci-shmem.o
OBJS += kvm-ipc.o
OBJS += builtin-sandbox.o
OBJS += virtio/mmio.o
+OBJS += virtio/notifier.o
# Translate uname -m into ARCH string
ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/)
diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index 750d30c..90ac622 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -33,6 +33,7 @@
#include "kvm/pci-shmem.h"
#include "kvm/kvm-ipc.h"
#include "kvm/builtin-debug.h"
+#include "kvm/virtio-notifier.h"
#include <linux/types.h>
#include <linux/err.h>
@@ -99,6 +100,7 @@ static bool using_rootfs;
static bool custom_rootfs;
static bool no_net;
static bool no_dhcp;
+static bool virtio_notifier;
extern bool ioport_debug;
extern bool mmio_debug;
static int kvm_run_wrapper;
@@ -442,6 +444,7 @@ static const struct option options[] = {
OPT_BOOLEAN('\0', "vnc", &vnc, "Enable VNC framebuffer"),
OPT_BOOLEAN('\0', "sdl", &sdl, "Enable SDL framebuffer"),
OPT_BOOLEAN('\0', "rng", &virtio_rng, "Enable virtio Random Number Generator"),
+ OPT_BOOLEAN('\0', "notifier", &virtio_notifier, "Enable virtio notifier"),
OPT_CALLBACK('\0', "9p", NULL, "dir_to_share,tag_name",
"Enable virtio 9p to share files between host and guest", virtio_9p_rootdir_parser),
OPT_STRING('\0', "console", &console, "serial, virtio or hv",
@@ -1182,6 +1185,9 @@ static int kvm_cmd_run_init(int argc, const char **argv)
if (virtio_rng)
virtio_rng__init(kvm);
+ if (virtio_notifier)
+ virtio_notif__init(kvm);
+
if (balloon)
virtio_bln__init(kvm);
diff --git a/tools/kvm/include/kvm/virtio-notifier.h b/tools/kvm/include/kvm/virtio-notifier.h
new file mode 100644
index 0000000..5673fc2
--- /dev/null
+++ b/tools/kvm/include/kvm/virtio-notifier.h
@@ -0,0 +1,9 @@
+#ifndef KVM__NOTIF_VIRTIO_H
+#define KVM__NOTIF_VIRTIO_H
+
+struct kvm;
+
+int virtio_notif__init(struct kvm *kvm);
+int virtio_notif__exit(struct kvm *kvm);
+
+#endif /* KVM__RNG_VIRTIO_H */
diff --git a/tools/kvm/include/kvm/virtio-pci-dev.h b/tools/kvm/include/kvm/virtio-pci-dev.h
index 7ceb125..a387ecd 100644
--- a/tools/kvm/include/kvm/virtio-pci-dev.h
+++ b/tools/kvm/include/kvm/virtio-pci-dev.h
@@ -13,6 +13,7 @@
#define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003
#define PCI_DEVICE_ID_VIRTIO_RNG 0x1004
#define PCI_DEVICE_ID_VIRTIO_BLN 0x1005
+#define PCI_DEVICE_ID_VIRTIO_NOTIFIER 0x1006
#define PCI_DEVICE_ID_VIRTIO_9P 0x1009
#define PCI_DEVICE_ID_VESA 0x2000
#define PCI_DEVICE_ID_PCI_SHMEM 0x0001
diff --git a/tools/kvm/virtio/notifier.c b/tools/kvm/virtio/notifier.c
new file mode 100644
index 0000000..c17546b
--- /dev/null
+++ b/tools/kvm/virtio/notifier.c
@@ -0,0 +1,203 @@
+#include "kvm/virtio-notifier.h"
+
+#include "kvm/virtio-pci-dev.h"
+
+#include "kvm/virtio.h"
+#include "kvm/util.h"
+#include "kvm/kvm.h"
+#include "kvm/threadpool.h"
+#include "kvm/guest_compat.h"
+
+#include <linux/virtio_ring.h>
+#include <linux/virtio_notifier.h>
+
+#include <linux/list.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <pthread.h>
+#include <linux/kernel.h>
+
+#define NUM_VIRT_QUEUES 2
+#define VIRTIO_NOTIFIER_QUEUE_SIZE 128
+
+struct ntf_dev_job {
+ struct virt_queue *vq;
+ struct ntf_dev *ndev;
+ struct thread_pool__job job_id;
+};
+
+struct ntf_dev {
+ struct list_head list;
+ struct virtio_device vdev;
+
+ /* virtio queue */
+ struct virt_queue vqs[NUM_VIRT_QUEUES];
+ struct ntf_dev_job jobs[NUM_VIRT_QUEUES];
+};
+
+static LIST_HEAD(ndevs);
+static int compat_id = -1;
+
+static void set_config(struct kvm *kvm, void *dev, u8 data, u32 offset)
+{
+ /* Unused */
+}
+
+static u8 get_config(struct kvm *kvm, void *dev, u32 offset)
+{
+ /* Unused */
+ return 0;
+}
+
+static u32 get_host_features(struct kvm *kvm, void *dev)
+{
+ /* Unused */
+ return 0;
+}
+
+static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
+{
+ /* Unused */
+}
+
+static bool virtio_ntf_do_io_request(struct kvm *kvm, struct ntf_dev *ndev, struct virt_queue *queue)
+{
+ struct iovec iov[VIRTIO_NOTIFIER_QUEUE_SIZE];
+ unsigned int len = 0;
+ u16 out, in, head;
+ u32 *counters;
+
+ head = virt_queue__get_iov(queue, iov, &out, &in, kvm);
+ counters = iov[0].iov_base;
+
+ printf("\n\n New updated counters from guest:\n\tPanic: %u OOM: %u\n\n",
+ counters[VIRTIO_NOTIF_PANIC], counters[VIRTIO_NOTIF_OOM]);
+
+ virt_queue__set_used_elem(queue, head, len);
+
+ return true;
+}
+
+static bool virtio_ntf_do_echo_request(struct kvm *kvm, struct ntf_dev *ndev, struct virt_queue *queue)
+{
+ struct iovec iov[VIRTIO_NOTIFIER_QUEUE_SIZE];
+ u16 out, in, head;
+
+ sleep(1);
+ head = virt_queue__get_iov(queue, iov, &out, &in, kvm);
+ virt_queue__set_used_elem(queue, head, iov[0].iov_len);
+
+ return true;
+}
+
+static void virtio_ntf_do_io(struct kvm *kvm, void *param)
+{
+ struct ntf_dev_job *job = param;
+ struct virt_queue *vq = job->vq;
+ struct ntf_dev *ndev = job->ndev;
+
+ if ((vq - ndev->vqs) == 0) {
+ while (virt_queue__available(vq))
+ virtio_ntf_do_io_request(kvm, ndev, vq);
+ } else {
+ virtio_ntf_do_echo_request(kvm, ndev, vq);
+ }
+
+ ndev->vdev.ops->signal_vq(kvm, &ndev->vdev, vq - ndev->vqs);
+}
+
+static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 pfn)
+{
+ struct ntf_dev *ndev = dev;
+ struct virt_queue *queue;
+ struct ntf_dev_job *job;
+ void *p;
+
+ compat__remove_message(compat_id);
+
+ queue = &ndev->vqs[vq];
+ queue->pfn = pfn;
+ p = guest_pfn_to_host(kvm, queue->pfn);
+
+ job = &ndev->jobs[vq];
+
+ vring_init(&queue->vring, VIRTIO_NOTIFIER_QUEUE_SIZE, p, VIRTIO_PCI_VRING_ALIGN);
+
+ *job = (struct ntf_dev_job) {
+ .vq = queue,
+ .ndev = ndev,
+ };
+
+ thread_pool__init_job(&job->job_id, kvm, virtio_ntf_do_io, job);
+
+ return 0;
+}
+
+static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ struct ntf_dev *ndev = dev;
+
+ thread_pool__do_job(&ndev->jobs[vq].job_id);
+
+ return 0;
+}
+
+static int get_pfn_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ struct ntf_dev *ndev = dev;
+
+ return ndev->vqs[vq].pfn;
+}
+
+static int get_size_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ return VIRTIO_NOTIFIER_QUEUE_SIZE;
+}
+
+static struct virtio_ops ntf_dev_virtio_ops = (struct virtio_ops) {
+ .set_config = set_config,
+ .get_config = get_config,
+ .get_host_features = get_host_features,
+ .set_guest_features = set_guest_features,
+ .init_vq = init_vq,
+ .notify_vq = notify_vq,
+ .get_pfn_vq = get_pfn_vq,
+ .get_size_vq = get_size_vq,
+};
+
+int virtio_notif__init(struct kvm *kvm)
+{
+ struct ntf_dev *ndev;
+ int r;
+
+ ndev = malloc(sizeof(*ndev));
+ if (ndev == NULL)
+ return -ENOMEM;
+
+ r = virtio_init(kvm, ndev, &ndev->vdev, &ntf_dev_virtio_ops,
+ VIRTIO_PCI, PCI_DEVICE_ID_VIRTIO_NOTIFIER, VIRTIO_ID_NOTIFIER, PCI_CLASS_RNG);
+ if (r < 0)
+ goto cleanup;
+
+ list_add_tail(&ndev->list, &ndevs);
+
+ return 0;
+cleanup:
+ free(ndev);
+
+ return r;
+}
+
+int virtio_notif__exit(struct kvm *kvm)
+{
+ struct ntf_dev *ndev, *tmp;
+
+ list_for_each_entry_safe(ndev, tmp, &ndevs, list) {
+ list_del(&ndev->list);
+ ndev->vdev.ops->exit(kvm, &ndev->vdev);
+ free(ndev);
+ }
+
+ return 0;
+}
--
1.7.8.6
^ permalink raw reply related
* [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Nicholas A. Bellinger @ 2012-07-24 1:26 UTC (permalink / raw)
To: target-devel
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
Greg Kroah-Hartman, qemu-devel, Zhi Yong Wu, Anthony Liguori,
linux-scsi, Paolo Bonzini, lf-virt, Christoph Hellwig
From: Nicholas Bellinger <nab@linux-iscsi.org>
As requested by Anthony, here is a patch against target-pending/for-next-merge
to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
ioctl operation.
As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.
I think this is what you had in mind, and hopefully it will make MST happy too.
The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
of cutting a new vhost-scsi RFC over the next days.
Please have a look and let me know if you have any concerns here.
Thanks!
Reported-by: Anthony Liguori <aliguori@us.ibm.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/vhost/tcm_vhost.c | 9 +++++++++
drivers/vhost/tcm_vhost.h | 11 +++++++++++
2 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index dc7e024..3f04169 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -977,6 +977,15 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
return -EFAULT;
return vhost_scsi_clear_endpoint(vs, &backend);
+ case VHOST_SCSI_GET_ABI_VERSION:
+ if (copy_from_user(&backend, argp, sizeof backend))
+ return -EFAULT;
+
+ backend.abi_version = VHOST_SCSI_ABI_VERSION;
+
+ if (copy_to_user(argp, &backend, sizeof backend))
+ return -EFAULT;
+ return 0;
case VHOST_GET_FEATURES:
features = VHOST_FEATURES;
if (copy_to_user(featurep, &features, sizeof features))
diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
index e942df9..3d5378f 100644
--- a/drivers/vhost/tcm_vhost.h
+++ b/drivers/vhost/tcm_vhost.h
@@ -80,7 +80,17 @@ struct tcm_vhost_tport {
#include <linux/vhost.h>
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
+ * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
+ */
+
+#define VHOST_SCSI_ABI_VERSION 1
+
struct vhost_scsi_target {
+ int abi_version;
unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
unsigned short vhost_tpgt;
};
@@ -88,3 +98,4 @@ struct vhost_scsi_target {
/* VHOST_SCSI specific defines */
#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
--
1.7.2.5
^ permalink raw reply related
* [PATCH 0/2] vhost-scsi: Check for tcm_vhost ABI version
From: Nicholas A. Bellinger @ 2012-07-24 1:30 UTC (permalink / raw)
To: target-devel
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
From: Nicholas Bellinger <nab@linux-iscsi.org>
Hi Anthony,
Here are the two patches against Zhi's vhost-scsi tree to check for
a supported version (VHOST_SCSI_ABI_VERSION=1) that's now exposed via
the tcm_vhost ioctl.
Please have a look and let me know if this is what you had in mind.
Thanks!
Nicholas Bellinger (2):
vhost-scsi: Rename vhost_vring_target -> vhost_scsi_target
vhost-scsi: Add support for VHOST_SCSI_GET_ABI_VERSION ioctl
hw/vhost-scsi.c | 21 +++++++++++++++++++--
hw/vhost-scsi.h | 17 ++++++++++++++---
2 files changed, 33 insertions(+), 5 deletions(-)
--
1.7.2.5
^ permalink raw reply
* [PATCH 1/2] vhost-scsi: Rename vhost_vring_target -> vhost_scsi_target
From: Nicholas A. Bellinger @ 2012-07-24 1:30 UTC (permalink / raw)
To: target-devel
Cc: Jens Axboe, Anthony Liguori, Stefan Hajnoczi, kvm-devel,
Michael S. Tsirkin, qemu-devel, Zhi Yong Wu, Anthony Liguori,
Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <1343093410-5843-1-git-send-email-nab@linux-iscsi.org>
From: Nicholas Bellinger <nab@linux-iscsi.org>
Rename the main IOCTL structure to vhost_scsi_target so that
it makes a little more sense than vhost_vring_target.
As requested by MST.
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
hw/vhost-scsi.c | 4 ++--
hw/vhost-scsi.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
index 3e3378a..3e9fa0e 100644
--- a/hw/vhost-scsi.c
+++ b/hw/vhost-scsi.c
@@ -49,7 +49,7 @@ const char *vhost_scsi_get_id(VHostSCSI *vs)
int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
{
int ret;
- struct vhost_vring_target backend;
+ struct vhost_scsi_target backend;
if (!vhost_dev_query(&vs->dev, vdev)) {
return -ENOTSUP;
@@ -86,7 +86,7 @@ void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev)
fprintf(stderr, "vhost_scsi_stop\n");
int ret;
- struct vhost_vring_target backend;
+ struct vhost_scsi_target backend;
pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
backend.vhost_tpgt = vs->tpgt;
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
index 84e9097..8ce974a 100644
--- a/hw/vhost-scsi.h
+++ b/hw/vhost-scsi.h
@@ -19,14 +19,14 @@
/* TODO #include <linux/vhost.h> properly */
/* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
-struct vhost_vring_target {
+struct vhost_scsi_target {
unsigned char vhost_wwpn[224];
unsigned short vhost_tpgt;
};
#define VHOST_VIRTIO 0xAF
-#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_vring_target)
-#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_vring_target)
+#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
+#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
VHostSCSI *find_vhost_scsi(const char *id);
const char *vhost_scsi_get_id(VHostSCSI *vs);
--
1.7.2.5
^ permalink raw reply related
* [PATCH 2/2] vhost-scsi: Add support for VHOST_SCSI_GET_ABI_VERSION ioctl
From: Nicholas A. Bellinger @ 2012-07-24 1:30 UTC (permalink / raw)
To: target-devel
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1343093410-5843-1-git-send-email-nab@linux-iscsi.org>
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch adds support in vhost-scsi QEMU userspace code to check the
running tcm_vhost ABI version using VHOST_SCSI_GET_ABI_VERSION.
This series selects VHOST_SCSI_ABI_VERSION=1 for current code, and
requires the pre-requsite tcm_vhost patch to expose an ABI version number.
This patch patch will fail to initialize vhost_scsi_start() unless both
vhost-scsi <-> tcm_vhost agree on VHOST_SCSI_ABI_VERSION=1.
Reported-by: Anthony Liguori <aliguori@us.ibm.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
hw/vhost-scsi.c | 17 +++++++++++++++++
hw/vhost-scsi.h | 11 +++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
index 3e9fa0e..9ab314c 100644
--- a/hw/vhost-scsi.c
+++ b/hw/vhost-scsi.c
@@ -68,6 +68,23 @@ int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
return ret;
}
+ memset(&backend, 0, sizeof(backend));
+ ret = ioctl(vs->dev.control, VHOST_SCSI_GET_ABI_VERSION, &backend);
+ if (ret < 0) {
+ ret = -errno;
+ vhost_dev_stop(&vs->dev, vdev);
+ return ret;
+ }
+ if (backend.abi_version > VHOST_SCSI_ABI_VERSION) {
+ fprintf(stderr, "The running tcm_vhost kernel abi_version: %d is greater"
+ " than vhost_scsi userspace supports: %d\n", backend.abi_version,
+ VHOST_SCSI_ABI_VERSION);
+ ret = -ENOSYS;
+ vhost_dev_stop(&vs->dev, vdev);
+ return ret;
+ }
+ printf("Using TCM_Vhost ABI version: %d\n", backend.abi_version);
+
pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
backend.vhost_tpgt = vs->tpgt;
ret = ioctl(vs->dev.control, VHOST_SCSI_SET_ENDPOINT, &backend);
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
index 8ce974a..393fe39 100644
--- a/hw/vhost-scsi.h
+++ b/hw/vhost-scsi.h
@@ -17,9 +17,19 @@
#include "qemu-common.h"
#include "qemu-option.h"
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
+ * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
+ */
+
+#define VHOST_SCSI_ABI_VERSION 1
+
/* TODO #include <linux/vhost.h> properly */
/* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
struct vhost_scsi_target {
+ int abi_version;
unsigned char vhost_wwpn[224];
unsigned short vhost_tpgt;
};
@@ -27,6 +37,7 @@ struct vhost_scsi_target {
#define VHOST_VIRTIO 0xAF
#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
VHostSCSI *find_vhost_scsi(const char *id);
const char *vhost_scsi_get_id(VHostSCSI *vs);
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Greg Kroah-Hartman @ 2012-07-24 1:56 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
qemu-devel, lf-virt, Anthony Liguori, target-devel, linux-scsi,
Paolo Bonzini, Zhi Yong Wu, Christoph Hellwig
In-Reply-To: <1343093180-5598-1-git-send-email-nab@linux-iscsi.org>
On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> As requested by Anthony, here is a patch against target-pending/for-next-merge
> to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
> ioctl operation.
>
> As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
> ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.
>
> I think this is what you had in mind, and hopefully it will make MST happy too.
> The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
> of cutting a new vhost-scsi RFC over the next days.
>
> Please have a look and let me know if you have any concerns here.
>
> Thanks!
>
> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/vhost/tcm_vhost.c | 9 +++++++++
> drivers/vhost/tcm_vhost.h | 11 +++++++++++
> 2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index dc7e024..3f04169 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -977,6 +977,15 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
> return -EFAULT;
>
> return vhost_scsi_clear_endpoint(vs, &backend);
> + case VHOST_SCSI_GET_ABI_VERSION:
> + if (copy_from_user(&backend, argp, sizeof backend))
> + return -EFAULT;
> +
> + backend.abi_version = VHOST_SCSI_ABI_VERSION;
> +
> + if (copy_to_user(argp, &backend, sizeof backend))
> + return -EFAULT;
> + return 0;
> case VHOST_GET_FEATURES:
> features = VHOST_FEATURES;
> if (copy_to_user(featurep, &features, sizeof features))
> diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> index e942df9..3d5378f 100644
> --- a/drivers/vhost/tcm_vhost.h
> +++ b/drivers/vhost/tcm_vhost.h
> @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
>
> #include <linux/vhost.h>
>
> +/*
> + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> + *
> + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> + */
> +
> +#define VHOST_SCSI_ABI_VERSION 1
> +
> struct vhost_scsi_target {
> + int abi_version;
> unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> unsigned short vhost_tpgt;
> };
> @@ -88,3 +98,4 @@ struct vhost_scsi_target {
> /* VHOST_SCSI specific defines */
> #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
No, you just broke the ABI for version "0" here, that's not how you do
this at all.
greg k-h
^ permalink raw reply
* [RFC PATCH 0/6] virtio-trace: Support virtio-trace
From: Yoshihiro YUNOMAE @ 2012-07-24 2:36 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Steven Rostedt, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah
Hi All,
The following patch set provides a low-overhead system for collecting kernel
tracing data of guests by a host in a virtualization environment.
A guest OS generally shares some devices with other guests or a host, so
reasons of any problems occurring in a guest may be from other guests or a host.
Then, to collect some tracing data of a number of guests and a host is needed
when some problems occur in a virtualization environment. One of methods to
realize that is to collect tracing data of guests in a host. To do this, network
is generally used. However, high load will be taken to applications on guests
using network I/O because there are many network stack layers. Therefore,
a communication method for collecting the data without using network is needed.
We submitted a patch set of "IVRing", a ring-buffer driver constructed on
Inter-VM shared memory (IVShmem), to LKML http://lwn.net/Articles/500304/ in
this June. IVRing and the IVRing reader use POSIX shared memory each other
without using network, so a low-overhead system for collecting guest tracing
data is realized. However, this patch set has some problems as follows:
- use IVShmem instead of virtio
- create a new ring-buffer without using existing ring-buffer in kernel
- scalability
-- not support SMP environment
-- buffer size limitation
-- not support live migration (maybe difficult for realize this)
Therefore, we propose a new system "virtio-trace", which uses enhanced
virtio-serial and existing ring-buffer of ftrace, for collecting guest kernel
tracing data. In this system, there are 5 main components:
(1) Ring-buffer of ftrace in a guest
- When trace agent reads ring-buffer, a page is removed from ring-buffer.
(2) Trace agent in the guest
- Splice the page of ring-buffer to read_pipe using splice() without
memory copying. Then, the page is spliced from write_pipe to virtio
without memory copying.
(3) Virtio-console driver in the guest
- Pass the page to virtio-ring
(4) Virtio-serial bus in QEMU
- Copy the page to kernel pipe
(5) Reader in the host
- Read guest tracing data via FIFO(named pipe)
***Evaluation***
When a host collects tracing data of a guest, the performance of using
virtio-trace is compared with that of using native(just running ftrace),
IVRing, and virtio-serial(normal method of read/write).
<environment>
The overview of this evaluation is as follows:
(a) A guest on a KVM is prepared.
- The guest is dedicated one physical CPU as a virtual CPU(VCPU).
(b) The guest starts to write tracing data to ring-buffer of ftrace.
- The probe points are all trace points of sched, timer, and kmem.
(c) Writing trace data, dhrystone 2 in UNIX bench is executed as a benchmark
tool in the guest.
- Dhrystone 2 intends system performance by repeating integer arithmetic
as a score.
- Since higher score equals to better system performance, if the score
decrease based on bare environment, it indicates that any operation
disturbs the integer arithmetic. Then, we define the overhead of
transporting trace data is calculated as follows:
OVERHEAD = (1 - SCORE_OF_A_METHOD/NATIVE_SCORE) * 100.
The performance of each method is compared as follows:
[1] Native
- only recording trace data to ring-buffer on a guest
[2] Virtio-trace
- running a trace agent on a guest
- a reader on a host opens FIFO using cat command
[3] IVRing
- A SystemTap script in a guest records trace data to IVRing.
-- probe points are same as ftrace.
[4] Virtio-serial(normal)
- A reader(using cat) on a guest output trace data to a host using
standard output via virtio-serial.
Other information is as follows:
- host
kernel: 3.3.7-1 (Fedora16)
CPU: Intel Xeon x5660@2.80GHz(12core)
Memory: 48GB
- guest(only booting one guest)
kernel: 3.5.0-rc4+ (Fedora16)
CPU: 1VCPU(dedicated)
Memory: 1GB
<result>
3 patterns based on the bare environment were indicated as follows:
Scores overhead against [0] Native
[0] Native: 28807569.5 -
[1] Virtio-trace: 28685049.5 0.43%
[2] IVRing: 28418595.5 1.35%
[3] Virtio-serial: 13262258.7 53.96%
***Just enhancement ideas***
- Support for trace-cmd
- Support for 9pfs protocol
- Support for non-blocking mode in QEMU
- Make "vhost-serial"
Thank you,
---
Masami Hiramatsu (5):
virtio/console: Allocate scatterlist according to the current pipe size
ftrace: Allow stealing pages from pipe buffer
virtio/console: Wait until the port is ready on splice
virtio/console: Add a failback for unstealable pipe buffer
virtio/console: Add splice_write support
Yoshihiro YUNOMAE (1):
tools: Add guest trace agent as a user tool
drivers/char/virtio_console.c | 198 ++++++++++++++++++--
kernel/trace/trace.c | 8 -
tools/virtio/virtio-trace/Makefile | 14 +
tools/virtio/virtio-trace/README | 118 ++++++++++++
tools/virtio/virtio-trace/trace-agent-ctl.c | 137 ++++++++++++++
tools/virtio/virtio-trace/trace-agent-rw.c | 192 +++++++++++++++++++
tools/virtio/virtio-trace/trace-agent.c | 270 +++++++++++++++++++++++++++
tools/virtio/virtio-trace/trace-agent.h | 75 ++++++++
8 files changed, 985 insertions(+), 27 deletions(-)
create mode 100644 tools/virtio/virtio-trace/Makefile
create mode 100644 tools/virtio/virtio-trace/README
create mode 100644 tools/virtio/virtio-trace/trace-agent-ctl.c
create mode 100644 tools/virtio/virtio-trace/trace-agent-rw.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.h
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com
^ permalink raw reply
* [RFC PATCH 1/6] virtio/console: Add splice_write support
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Enable to use splice_write from pipe to virtio-console port.
This steals pages from pipe and directly send it to host.
Note that this may accelerate only the guest to host path.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/virtio_console.c | 136 +++++++++++++++++++++++++++++++++++++++--
1 files changed, 128 insertions(+), 8 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index cdf2f54..fe31b2f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -24,6 +24,8 @@
#include <linux/err.h>
#include <linux/freezer.h>
#include <linux/fs.h>
+#include <linux/splice.h>
+#include <linux/pagemap.h>
#include <linux/init.h>
#include <linux/list.h>
#include <linux/poll.h>
@@ -227,6 +229,7 @@ struct port {
bool guest_connected;
};
+#define MAX_SPLICE_PAGES 32
/* This is the very early arch-specified put chars function. */
static int (*early_put_chars)(u32, const char *, int);
@@ -474,26 +477,52 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
return 0;
}
+struct buffer_token {
+ union {
+ void *buf;
+ struct scatterlist *sg;
+ } u;
+ bool sgpages;
+};
+
+static void reclaim_sg_pages(struct scatterlist *sg)
+{
+ int i;
+ struct page *page;
+
+ for (i = 0; i < MAX_SPLICE_PAGES; i++) {
+ page = sg_page(&sg[i]);
+ if (!page)
+ break;
+ put_page(page);
+ }
+ kfree(sg);
+}
+
/* Callers must take the port->outvq_lock */
static void reclaim_consumed_buffers(struct port *port)
{
- void *buf;
+ struct buffer_token *tok;
unsigned int len;
if (!port->portdev) {
/* Device has been unplugged. vqs are already gone. */
return;
}
- while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
- kfree(buf);
+ while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
+ if (tok->sgpages)
+ reclaim_sg_pages(tok->u.sg);
+ else
+ kfree(tok->u.buf);
+ kfree(tok);
port->outvq_full = false;
}
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
- bool nonblock)
+static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
+ int nents, size_t in_count,
+ struct buffer_token *tok, bool nonblock)
{
- struct scatterlist sg[1];
struct virtqueue *out_vq;
ssize_t ret;
unsigned long flags;
@@ -505,8 +534,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
reclaim_consumed_buffers(port);
- sg_init_one(sg, in_buf, in_count);
- ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
+ ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -544,6 +572,37 @@ done:
return in_count;
}
+static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
+ bool nonblock)
+{
+ struct scatterlist sg[1];
+ struct buffer_token *tok;
+
+ tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
+ if (!tok)
+ return -ENOMEM;
+ tok->sgpages = false;
+ tok->u.buf = in_buf;
+
+ sg_init_one(sg, in_buf, in_count);
+
+ return __send_to_port(port, sg, 1, in_count, tok, nonblock);
+}
+
+static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
+ size_t in_count, bool nonblock)
+{
+ struct buffer_token *tok;
+
+ tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
+ if (!tok)
+ return -ENOMEM;
+ tok->sgpages = true;
+ tok->u.sg = sg;
+
+ return __send_to_port(port, sg, nents, in_count, tok, nonblock);
+}
+
/*
* Give out the data that's requested from the buffer that we have
* queued up.
@@ -725,6 +784,66 @@ out:
return ret;
}
+struct sg_list {
+ unsigned int n;
+ size_t len;
+ struct scatterlist *sg;
+};
+
+static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
+ struct splice_desc *sd)
+{
+ struct sg_list *sgl = sd->u.data;
+ unsigned int len = 0;
+
+ if (sgl->n == MAX_SPLICE_PAGES)
+ return 0;
+
+ /* Try lock this page */
+ if (buf->ops->steal(pipe, buf) == 0) {
+ /* Get reference and unlock page for moving */
+ get_page(buf->page);
+ unlock_page(buf->page);
+
+ len = min(buf->len, sd->len);
+ sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
+ sgl->n++;
+ sgl->len += len;
+ }
+
+ return len;
+}
+
+/* Faster zero-copy write by splicing */
+static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
+ struct file *filp, loff_t *ppos,
+ size_t len, unsigned int flags)
+{
+ struct port *port = filp->private_data;
+ struct sg_list sgl;
+ ssize_t ret;
+ struct splice_desc sd = {
+ .total_len = len,
+ .flags = flags,
+ .pos = *ppos,
+ .u.data = &sgl,
+ };
+
+ sgl.n = 0;
+ sgl.len = 0;
+ sgl.sg = kmalloc(sizeof(struct scatterlist) * MAX_SPLICE_PAGES,
+ GFP_ATOMIC);
+ if (unlikely(!sgl.sg))
+ return -ENOMEM;
+
+ sg_init_table(sgl.sg, MAX_SPLICE_PAGES);
+ ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
+ if (likely(ret > 0))
+ ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
+
+ return ret;
+}
+
static unsigned int port_fops_poll(struct file *filp, poll_table *wait)
{
struct port *port;
@@ -856,6 +975,7 @@ static const struct file_operations port_fops = {
.open = port_fops_open,
.read = port_fops_read,
.write = port_fops_write,
+ .splice_write = port_fops_splice_write,
.poll = port_fops_poll,
.release = port_fops_release,
.fasync = port_fops_fasync,
^ permalink raw reply related
* [RFC PATCH 2/6] virtio/console: Add a failback for unstealable pipe buffer
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Add a failback memcpy path for unstealable pipe buffer.
If buf->ops->steal() fails, virtio-serial tries to
copy the page contents to an allocated page, instead
of just failing splice().
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/virtio_console.c | 28 +++++++++++++++++++++++++---
1 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fe31b2f..911cb3e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
struct splice_desc *sd)
{
struct sg_list *sgl = sd->u.data;
- unsigned int len = 0;
+ unsigned int offset, len;
if (sgl->n == MAX_SPLICE_PAGES)
return 0;
@@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
len = min(buf->len, sd->len);
sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
- sgl->n++;
- sgl->len += len;
+ } else {
+ /* Failback to copying a page */
+ struct page *page = alloc_page(GFP_KERNEL);
+ char *src = buf->ops->map(pipe, buf, 1);
+ char *dst;
+
+ if (!page)
+ return -ENOMEM;
+ dst = kmap(page);
+
+ offset = sd->pos & ~PAGE_MASK;
+
+ len = sd->len;
+ if (len + offset > PAGE_SIZE)
+ len = PAGE_SIZE - offset;
+
+ memcpy(dst + offset, src + buf->offset, len);
+
+ kunmap(page);
+ buf->ops->unmap(pipe, buf, src);
+
+ sg_set_page(&(sgl->sg[sgl->n]), page, len, offset);
}
+ sgl->n++;
+ sgl->len += len;
return len;
}
^ permalink raw reply related
* [RFC PATCH 3/6] virtio/console: Wait until the port is ready on splice
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Wait if the port is not connected or full on splice
like as write is doing.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/virtio_console.c | 39 +++++++++++++++++++++++++++------------
1 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 911cb3e..e49d435 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -724,6 +724,26 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
return fill_readbuf(port, ubuf, count, true);
}
+static int wait_port_writable(struct port *port, bool nonblock)
+{
+ int ret;
+
+ if (will_write_block(port)) {
+ if (nonblock)
+ return -EAGAIN;
+
+ ret = wait_event_freezable(port->waitqueue,
+ !will_write_block(port));
+ if (ret < 0)
+ return ret;
+ }
+ /* Port got hot-unplugged. */
+ if (!port->guest_connected)
+ return -ENODEV;
+
+ return 0;
+}
+
static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
size_t count, loff_t *offp)
{
@@ -740,18 +760,9 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
nonblock = filp->f_flags & O_NONBLOCK;
- if (will_write_block(port)) {
- if (nonblock)
- return -EAGAIN;
-
- ret = wait_event_freezable(port->waitqueue,
- !will_write_block(port));
- if (ret < 0)
- return ret;
- }
- /* Port got hot-unplugged. */
- if (!port->guest_connected)
- return -ENODEV;
+ ret = wait_port_writable(port, nonblock);
+ if (ret < 0)
+ return ret;
count = min((size_t)(32 * 1024), count);
@@ -851,6 +862,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
.u.data = &sgl,
};
+ ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
+ if (ret < 0)
+ return ret;
+
sgl.n = 0;
sgl.len = 0;
sgl.sg = kmalloc(sizeof(struct scatterlist) * MAX_SPLICE_PAGES,
^ permalink raw reply related
* [RFC PATCH 4/6] ftrace: Allow stealing pages from pipe buffer
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Use generic steal operation on pipe buffer to allow stealing
ring buffer's read page from pipe buffer.
Note that this could reduce the performance of splice on the
splice_write side operation without affinity setting.
Since the ring buffer's read pages are allocated on the
tracing-node, but the splice user does not always execute
splice write side operation on the same node. In this case,
the page will be accessed from the another node.
Thus, it is strongly recommended to assign the splicing
thread to corresponding node.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
---
kernel/trace/trace.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a120f98..ae01930 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4194,12 +4194,6 @@ static void buffer_pipe_buf_release(struct pipe_inode_info *pipe,
buf->private = 0;
}
-static int buffer_pipe_buf_steal(struct pipe_inode_info *pipe,
- struct pipe_buffer *buf)
-{
- return 1;
-}
-
static void buffer_pipe_buf_get(struct pipe_inode_info *pipe,
struct pipe_buffer *buf)
{
@@ -4215,7 +4209,7 @@ static const struct pipe_buf_operations buffer_pipe_buf_ops = {
.unmap = generic_pipe_buf_unmap,
.confirm = generic_pipe_buf_confirm,
.release = buffer_pipe_buf_release,
- .steal = buffer_pipe_buf_steal,
+ .steal = generic_pipe_buf_steal,
.get = buffer_pipe_buf_get,
};
^ permalink raw reply related
* [RFC PATCH 5/6] virtio/console: Allocate scatterlist according to the current pipe size
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Allocate scatterlist according to the current pipe size.
This allows splicing bigger buffer if the pipe size has
been changed by fcntl.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/virtio_console.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e49d435..f5063d5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -229,7 +229,6 @@ struct port {
bool guest_connected;
};
-#define MAX_SPLICE_PAGES 32
/* This is the very early arch-specified put chars function. */
static int (*early_put_chars)(u32, const char *, int);
@@ -482,15 +481,16 @@ struct buffer_token {
void *buf;
struct scatterlist *sg;
} u;
- bool sgpages;
+ /* If sgpages == 0 then buf is used, else sg is used */
+ unsigned int sgpages;
};
-static void reclaim_sg_pages(struct scatterlist *sg)
+static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
{
int i;
struct page *page;
- for (i = 0; i < MAX_SPLICE_PAGES; i++) {
+ for (i = 0; i < nrpages; i++) {
page = sg_page(&sg[i]);
if (!page)
break;
@@ -511,7 +511,7 @@ static void reclaim_consumed_buffers(struct port *port)
}
while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
if (tok->sgpages)
- reclaim_sg_pages(tok->u.sg);
+ reclaim_sg_pages(tok->u.sg, tok->sgpages);
else
kfree(tok->u.buf);
kfree(tok);
@@ -581,7 +581,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
if (!tok)
return -ENOMEM;
- tok->sgpages = false;
+ tok->sgpages = 0;
tok->u.buf = in_buf;
sg_init_one(sg, in_buf, in_count);
@@ -597,7 +597,7 @@ static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
if (!tok)
return -ENOMEM;
- tok->sgpages = true;
+ tok->sgpages = nents;
tok->u.sg = sg;
return __send_to_port(port, sg, nents, in_count, tok, nonblock);
@@ -797,6 +797,7 @@ out:
struct sg_list {
unsigned int n;
+ unsigned int size;
size_t len;
struct scatterlist *sg;
};
@@ -807,7 +808,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
struct sg_list *sgl = sd->u.data;
unsigned int offset, len;
- if (sgl->n == MAX_SPLICE_PAGES)
+ if (sgl->n == sgl->size)
return 0;
/* Try lock this page */
@@ -868,12 +869,12 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
sgl.n = 0;
sgl.len = 0;
- sgl.sg = kmalloc(sizeof(struct scatterlist) * MAX_SPLICE_PAGES,
- GFP_ATOMIC);
+ sgl.size = pipe->nrbufs;
+ sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_ATOMIC);
if (unlikely(!sgl.sg))
return -ENOMEM;
- sg_init_table(sgl.sg, MAX_SPLICE_PAGES);
+ sg_init_table(sgl.sg, sgl.size);
ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
if (likely(ret > 0))
ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
^ permalink raw reply related
* [RFC PATCH 6/6] tools: Add guest trace agent as a user tool
From: Yoshihiro YUNOMAE @ 2012-07-24 2:37 UTC (permalink / raw)
To: linux-kernel
Cc: Herbert Xu, Arnd Bergmann, Frederic Weisbecker, yrl.pp-manager.tt,
qemu-devel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Steven Rostedt, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah
In-Reply-To: <20120724023657.6600.52706.stgit@ltc189.sdl.hitachi.co.jp>
This patch adds a user tool, "trace agent" for sending trace data of a guest to
a Host in low overhead. This agent has the following functions:
- splice a page of ring-buffer to read_pipe without memory copying
- splice the page from write_pipe to virtio-console without memory copying
- write trace data to stdout by using -o option
- controlled by start/stop orders from a Host
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
tools/virtio/virtio-trace/Makefile | 14 +
tools/virtio/virtio-trace/README | 118 ++++++++++++
tools/virtio/virtio-trace/trace-agent-ctl.c | 137 ++++++++++++++
tools/virtio/virtio-trace/trace-agent-rw.c | 192 +++++++++++++++++++
tools/virtio/virtio-trace/trace-agent.c | 270 +++++++++++++++++++++++++++
tools/virtio/virtio-trace/trace-agent.h | 75 ++++++++
6 files changed, 806 insertions(+), 0 deletions(-)
create mode 100644 tools/virtio/virtio-trace/Makefile
create mode 100644 tools/virtio/virtio-trace/README
create mode 100644 tools/virtio/virtio-trace/trace-agent-ctl.c
create mode 100644 tools/virtio/virtio-trace/trace-agent-rw.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.h
diff --git a/tools/virtio/virtio-trace/Makefile b/tools/virtio/virtio-trace/Makefile
new file mode 100644
index 0000000..ef3adfc
--- /dev/null
+++ b/tools/virtio/virtio-trace/Makefile
@@ -0,0 +1,14 @@
+CC = gcc
+CFLAGS = -O2 -Wall
+LFLAG = -lpthread
+
+all: trace-agent
+
+.c.o:
+ $(CC) $(CFLAGS) $(LFLAG) -c $^ -o $@
+
+trace-agent: trace-agent.o trace-agent-ctl.o trace-agent-rw.o
+ $(CC) $(CFLAGS) $(LFLAG) -o $@ $^
+
+clean:
+ rm -f *.o trace-agent
diff --git a/tools/virtio/virtio-trace/README b/tools/virtio/virtio-trace/README
new file mode 100644
index 0000000..b64845b
--- /dev/null
+++ b/tools/virtio/virtio-trace/README
@@ -0,0 +1,118 @@
+Trace Agent for virtio-trace
+============================
+
+Trace agent is a user tool for sending trace data of a guest to a Host in low
+overhead. Trace agent has the following functions:
+ - splice a page of ring-buffer to read_pipe without memory copying
+ - splice the page from write_pipe to virtio-console without memory copying
+ - write trace data to stdout by using -o option
+ - controlled by start/stop orders from a Host
+
+The trace agent operates as follows:
+ 1) Initialize all structures.
+ 2) Create a read/write thread per CPU. Each thread is bound to a CPU.
+ The read/write threads hold it.
+ 3) A controller thread does poll() for a start order of a host.
+ 4) After the controller of the trace agent receives a start order from a host,
+ the controller wake read/write threads.
+ 5) The read/write threads start to read trace data from ring-buffers and
+ write the data to virtio-serial.
+ 6) If the controller receives a stop order from a host, the read/write threads
+ stop to read trace data.
+
+
+Files
+=====
+
+README: this file
+Makefile: Makefile of trace agent for virtio-trace
+trace-agent.c: includes main function, sets up for operating trace agent
+trace-agent.h: includes all structures and some macros
+trace-agent-ctl.c: includes controller function for read/write threads
+trace-agent-rw.c: includes read/write threads function
+
+
+Setup
+=====
+
+To use this trace agent for virtio-trace, we need to prepare some virtio-serial
+I/Fs.
+
+1) Make FIFO in a host
+ virtio-trace uses virtio-serial pipe as trace data paths as to the number
+of CPUs and a control path, so FIFO (named pipe) should be created as follows:
+ # mkdir /tmp/virtio-trace/
+ # mkfifo /tmp/virtio-trace/trace-path-cpu{0,1,2,...,X}.{in,out}
+ # mkfifo /tmp/virtio-trace/agent-ctl-path.{in,out}
+
+For example, if a guest use three CPUs, the names are
+ trace-path-cpu{0,1,2}.{in.out}
+and
+ agent-ctl-path.{in,out}.
+
+2) Set up of virtio-serial pipe in a host
+ Add qemu option to use virtio-serial pipe.
+
+ ##virtio-serial device##
+ -device virtio-serial-pci,id=virtio-serial0\
+ ##control path##
+ -chardev pipe,id=charchannel0,path=/tmp/virtio-trace/agent-ctl-path\
+ -device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
+ id=channel0,name=agent-ctl-path\
+ ##data path##
+ -chardev pipe,id=charchannel1,path=/tmp/virtio-trace/trace-path-cpu0\
+ -device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel0,\
+ id=channel1,name=trace-path-cpu0\
+ ...
+
+If you manage guests with libvirt, add the following tags to domain XML files.
+Then, libvirt passes the same command option to qemu.
+
+ <channel type='pipe'>
+ <source path='/tmp/virtio-trace/agent-ctl-path'/>
+ <target type='virtio' name='agent-ctl-path'/>
+ <address type='virtio-serial' controller='0' bus='0' port='0'/>
+ </channel>
+ <channel type='pipe'>
+ <source path='/tmp/virtio-trace/trace-path-cpu0'/>
+ <target type='virtio' name='trace-path-cpu0'/>
+ <address type='virtio-serial' controller='0' bus='0' port='1'/>
+ </channel>
+ ...
+Here, chardev names are restricted to trace-path-cpuX and agent-ctl-path. For
+example, if a guest use three CPUs, chardev names should be trace-path-cpu0,
+trace-path-cpu1, trace-path-cpu2, and agent-ctl-path.
+
+3) Boot the guest
+ You can find some chardev in /dev/virtio-ports/ in the guest.
+
+
+Run
+===
+
+0) Build trace agent in a guest
+ $ make
+
+1) Enable ftrace in the guest
+ <Example>
+ # echo 1 > /sys/kernel/debug/tracing/events/sched/enable
+
+2) Run trace agent in the guest
+ This agent must be operated as root.
+ # ./trace-agent
+read/write threads in the agent wait for start order from host. If you add -o
+option, trace data are output via stdout in the guest.
+
+3) Open FIFO in a host
+ # cat /tmp/virtio-trace/trace-path-cpu0.out
+If a host does not open these, trace data get stuck in buffers of virtio. Then,
+the guest will stop by specification of chardev in QEMU. This blocking mode may
+be solved in the future.
+
+4) Start to read trace data by ordering from a host
+ A host injects read start order to the guest via virtio-serial.
+ # echo 1 > /tmp/virtio-trace/agent-ctl-path.in
+
+5) Stop to read trace data by ordering from a host
+ A host injects read stop order to the guest via virtio-serial.
+ # echo 0 > /tmp/virtio-trace/agent-ctl-path.in
diff --git a/tools/virtio/virtio-trace/trace-agent-ctl.c b/tools/virtio/virtio-trace/trace-agent-ctl.c
new file mode 100644
index 0000000..0739815
--- /dev/null
+++ b/tools/virtio/virtio-trace/trace-agent-ctl.c
@@ -0,0 +1,137 @@
+/*
+ * Controller of read/write threads for virtio-trace
+ *
+ * Copyright (C) 2012 Hitachi, Ltd.
+ * Created by Yoshihiro Yunomae <yoshihiro.yunomae.ez@hitachi.com>
+ * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
+ *
+ * Licensed under GPL version 2 only.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <poll.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "trace-agent.h"
+
+#define HOST_MSG_SIZE 256
+#define EVENT_WAIT_MSEC 100
+
+static volatile sig_atomic_t global_signal_val;
+bool global_sig_receive; /* default false */
+bool global_run_operation; /* default false*/
+
+/* Handle SIGTERM/SIGINT/SIGQUIT to exit */
+static void signal_handler(int sig)
+{
+ global_signal_val = sig;
+}
+
+int rw_ctl_init(const char *ctl_path)
+{
+ int ctl_fd;
+
+ ctl_fd = open(ctl_path, O_RDONLY);
+ if (ctl_fd == -1) {
+ fprintf(stderr, "Cannot open ctl_fd\n");
+ goto error;
+ }
+
+ return ctl_fd;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+static int wait_order(int ctl_fd)
+{
+ struct pollfd poll_fd;
+ int ret = 0;
+
+ while (!global_sig_receive) {
+ poll_fd.fd = ctl_fd;
+ poll_fd.events = POLLIN;
+
+ ret = poll(&poll_fd, 1, EVENT_WAIT_MSEC);
+
+ if (global_signal_val) {
+ global_sig_receive = true;
+ pr_info("Receive interrupt %d\n", global_signal_val);
+
+ /* Wakes rw-threads when they are sleeping */
+ if (!global_run_operation)
+ pthread_cond_broadcast(&cond_wakeup);
+
+ ret = -1;
+ break;
+ }
+
+ if (ret < 0) {
+ pr_err("Polling error\n");
+ goto error;
+ }
+
+ if (ret)
+ break;
+ };
+
+ return ret;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+/*
+ * contol read/write threads by handling global_run_operation
+ */
+void *rw_ctl_loop(int ctl_fd)
+{
+ ssize_t rlen;
+ char buf[HOST_MSG_SIZE];
+ int ret;
+
+ /* Setup signal handlers */
+ signal(SIGTERM, signal_handler);
+ signal(SIGINT, signal_handler);
+ signal(SIGQUIT, signal_handler);
+
+ while (!global_sig_receive) {
+
+ ret = wait_order(ctl_fd);
+ if (ret < 0)
+ break;
+
+ rlen = read(ctl_fd, buf, sizeof(buf));
+ if (rlen < 0) {
+ pr_err("read data error in ctl thread\n");
+ goto error;
+ }
+
+ if (rlen == 2 && buf[0] == '1') {
+ /*
+ * If host writes '1' to a control path,
+ * this controller wakes all read/write threads.
+ */
+ global_run_operation = true;
+ pthread_cond_broadcast(&cond_wakeup);
+ pr_debug("Wake up all read/write threads\n");
+ } else if (rlen == 2 && buf[0] == '0') {
+ /*
+ * If host writes '0' to a control path, read/write
+ * threads will wait for notification from Host.
+ */
+ global_run_operation = false;
+ pr_debug("Stop all read/write threads\n");
+ } else
+ pr_info("Invalid host notification: %s\n", buf);
+ }
+
+ return NULL;
+
+error:
+ exit(EXIT_FAILURE);
+}
diff --git a/tools/virtio/virtio-trace/trace-agent-rw.c b/tools/virtio/virtio-trace/trace-agent-rw.c
new file mode 100644
index 0000000..3aace5e
--- /dev/null
+++ b/tools/virtio/virtio-trace/trace-agent-rw.c
@@ -0,0 +1,192 @@
+/*
+ * Read/write thread of a guest agent for virtio-trace
+ *
+ * Copyright (C) 2012 Hitachi, Ltd.
+ * Created by Yoshihiro Yunomae <yoshihiro.yunomae.ez@hitachi.com>
+ * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
+ *
+ * Licensed under GPL version 2 only.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include "trace-agent.h"
+
+#define READ_WAIT_USEC 100000
+
+void *rw_thread_info_new(void)
+{
+ struct rw_thread_info *rw_ti;
+
+ rw_ti = zalloc(sizeof(struct rw_thread_info));
+ if (rw_ti == NULL) {
+ pr_err("rw_thread_info zalloc error\n");
+ exit(EXIT_FAILURE);
+ }
+
+ rw_ti->cpu_num = -1;
+ rw_ti->in_fd = -1;
+ rw_ti->out_fd = -1;
+ rw_ti->read_pipe = -1;
+ rw_ti->write_pipe = -1;
+ rw_ti->pipe_size = PIPE_INIT;
+
+ return rw_ti;
+}
+
+void *rw_thread_init(int cpu, const char *in_path, const char *out_path,
+ bool stdout_flag, unsigned long pipe_size,
+ struct rw_thread_info *rw_ti)
+{
+ int data_pipe[2];
+
+ rw_ti->cpu_num = cpu;
+
+ /* set read(input) fd */
+ rw_ti->in_fd = open(in_path, O_RDONLY);
+ if (rw_ti->in_fd == -1) {
+ pr_err("Could not open in_fd (CPU:%d)\n", cpu);
+ goto error;
+ }
+
+ /* set write(output) fd */
+ if (!stdout_flag) {
+ /* virtio-serial output mode */
+ rw_ti->out_fd = open(out_path, O_WRONLY);
+ if (rw_ti->out_fd == -1) {
+ pr_err("Could not open out_fd (CPU:%d)\n", cpu);
+ goto error;
+ }
+ } else
+ /* stdout mode */
+ rw_ti->out_fd = STDOUT_FILENO;
+
+ if (pipe2(data_pipe, O_NONBLOCK) < 0) {
+ pr_err("Could not create pipe in rw-thread(%d)\n", cpu);
+ goto error;
+ }
+
+ /*
+ * Size of pipe is 64kB in default based on fs/pipe.c.
+ * To read/write trace data speedy, pipe size is changed.
+ */
+ if (fcntl(*data_pipe, F_SETPIPE_SZ, pipe_size) < 0) {
+ pr_err("Could not change pipe size in rw-thread(%d)\n", cpu);
+ goto error;
+ }
+
+ rw_ti->read_pipe = data_pipe[1];
+ rw_ti->write_pipe = data_pipe[0];
+ rw_ti->pipe_size = pipe_size;
+
+ return NULL;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+/* Bind a thread to a cpu */
+static void bind_cpu(int cpu_num)
+{
+ cpu_set_t mask;
+
+ CPU_ZERO(&mask);
+ CPU_SET(cpu_num, &mask);
+
+ /* bind my thread to cpu_num by assigning zero to the first argument */
+ if (sched_setaffinity(0, sizeof(mask), &mask) == -1)
+ pr_err("Could not set CPU#%d affinity\n", (int)cpu_num);
+}
+
+static void *rw_thread_main(void *thread_info)
+{
+ ssize_t rlen, wlen;
+ ssize_t ret;
+ struct rw_thread_info *ts = (struct rw_thread_info *)thread_info;
+
+ bind_cpu(ts->cpu_num);
+
+ while (1) {
+ /* Wait for a read order of trace data by Host OS */
+ if (!global_run_operation) {
+ pthread_mutex_lock(&mutex_notify);
+ pthread_cond_wait(&cond_wakeup, &mutex_notify);
+ pthread_mutex_unlock(&mutex_notify);
+ }
+
+ if (global_sig_receive)
+ break;
+
+ /*
+ * Each thread read trace_pipe_raw of each cpu bounding the
+ * thread, so contention of multi-threads does not occur.
+ */
+ rlen = splice(ts->in_fd, NULL, ts->read_pipe, NULL,
+ ts->pipe_size, SPLICE_F_MOVE | SPLICE_F_MORE);
+
+ if (rlen < 0) {
+ pr_err("Splice_read in rw-thread(%d)\n", ts->cpu_num);
+ goto error;
+ } else if (rlen == 0) {
+ /*
+ * If trace data do not exist or are unreadable not
+ * for exceeding the page size, splice_read returns
+ * NULL. Then, this waits for being filled the data in a
+ * ring-buffer.
+ */
+ usleep(READ_WAIT_USEC);
+ pr_debug("Read retry(cpu:%d)\n", ts->cpu_num);
+ continue;
+ }
+
+ wlen = 0;
+
+ do {
+ ret = splice(ts->write_pipe, NULL, ts->out_fd, NULL,
+ rlen - wlen,
+ SPLICE_F_MOVE | SPLICE_F_MORE);
+
+ if (ret < 0) {
+ pr_err("Splice_write in rw-thread(%d)\n",
+ ts->cpu_num);
+ goto error;
+ } else if (ret == 0)
+ /*
+ * When host reader is not in time for reading
+ * trace data, guest will be stopped. This is
+ * because char dev in QEMU is not supported
+ * non-blocking mode. Then, writer might be
+ * sleep in that case.
+ * This sleep will be removed by supporting
+ * non-blocking mode.
+ */
+ sleep(1);
+ wlen += ret;
+ } while (wlen < rlen);
+ }
+
+ return NULL;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+
+pthread_t rw_thread_run(struct rw_thread_info *rw_ti)
+{
+ int ret;
+ pthread_t rw_thread_per_cpu;
+
+ ret = pthread_create(&rw_thread_per_cpu, NULL, rw_thread_main, rw_ti);
+ if (ret != 0) {
+ pr_err("Could not create a rw thread(%d)\n", rw_ti->cpu_num);
+ exit(EXIT_FAILURE);
+ }
+
+ return rw_thread_per_cpu;
+}
diff --git a/tools/virtio/virtio-trace/trace-agent.c b/tools/virtio/virtio-trace/trace-agent.c
new file mode 100644
index 0000000..1816259
--- /dev/null
+++ b/tools/virtio/virtio-trace/trace-agent.c
@@ -0,0 +1,270 @@
+/*
+ * Guest agent for virtio-trace
+ *
+ * Copyright (C) 2012 Hitachi, Ltd.
+ * Created by Yoshihiro Yunomae <yoshihiro.yunomae.ez@hitachi.com>
+ * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
+ *
+ * Licensed under GPL version 2 only.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "trace-agent.h"
+
+#define PAGE_SIZE (sysconf(_SC_PAGE_SIZE))
+#define PIPE_DEF_BUFS 16
+#define PIPE_MIN_SIZE (PAGE_SIZE*PIPE_DEF_BUFS)
+#define PIPE_MAX_SIZE (1024*1024)
+#define READ_PATH_FMT \
+ "/sys/kernel/debug/tracing/per_cpu/cpu%d/trace_pipe_raw"
+#define WRITE_PATH_FMT "/dev/virtio-ports/trace-path-cpu%d"
+#define CTL_PATH "/dev/virtio-ports/agent-ctl-path"
+
+pthread_mutex_t mutex_notify = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t cond_wakeup = PTHREAD_COND_INITIALIZER;
+
+static int get_total_cpus(void)
+{
+ int nr_cpus = (int)sysconf(_SC_NPROCESSORS_CONF);
+
+ if (nr_cpus <= 0) {
+ pr_err("Could not read cpus\n");
+ goto error;
+ } else if (nr_cpus > MAX_CPUS) {
+ pr_err("Exceed max cpus(%d)\n", (int)MAX_CPUS);
+ goto error;
+ }
+
+ return nr_cpus;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+static void *agent_info_new(void)
+{
+ struct agent_info *s;
+ int i;
+
+ s = zalloc(sizeof(struct agent_info));
+ if (s == NULL) {
+ pr_err("agent_info zalloc error\n");
+ exit(EXIT_FAILURE);
+ }
+
+ s->pipe_size = PIPE_INIT;
+ s->use_stdout = false;
+ s->cpus = get_total_cpus();
+ s->ctl_fd = -1;
+
+ /* read/write threads init */
+ for (i = 0; i < s->cpus; i++)
+ s->rw_ti[i] = rw_thread_info_new();
+
+ return s;
+}
+
+static unsigned long parse_size(const char *arg)
+{
+ unsigned long value, round;
+ char *ptr;
+
+ value = strtoul(arg, &ptr, 10);
+ switch (*ptr) {
+ case 'K': case 'k':
+ value <<= 10;
+ break;
+ case 'M': case 'm':
+ value <<= 20;
+ break;
+ default:
+ break;
+ }
+
+ if (value > PIPE_MAX_SIZE) {
+ pr_err("Pipe size must be less than 1MB\n");
+ goto error;
+ } else if (value < PIPE_MIN_SIZE) {
+ pr_err("Pipe size must be over 64KB\n");
+ goto error;
+ }
+
+ /* Align buffer size with page unit */
+ round = value & (PAGE_SIZE - 1);
+ value = value - round;
+
+ return value;
+error:
+ return 0;
+}
+
+static void usage(char const *prg)
+{
+ fprintf(stderr, "usage: %s [-h] [-o] [-s <size of pipe>]\n", prg);
+}
+
+static const char *make_path(int cpu_num, bool this_is_write_path)
+{
+ int ret;
+ char *buf;
+
+ buf = zalloc(PATH_MAX);
+ if (buf == NULL) {
+ pr_err("Could not allocate buffer\n");
+ goto error;
+ }
+
+ if (this_is_write_path)
+ /* write(output) path */
+ ret = snprintf(buf, PATH_MAX, WRITE_PATH_FMT, cpu_num);
+ else
+ /* read(input) path */
+ ret = snprintf(buf, PATH_MAX, READ_PATH_FMT, cpu_num);
+
+ if (ret <= 0) {
+ pr_err("Failed to generate %s path(CPU#%d):%d\n",
+ this_is_write_path ? "read" : "write", cpu_num, ret);
+ goto error;
+ }
+
+ return buf;
+
+error:
+ free(buf);
+ return NULL;
+}
+
+static const char *make_input_path(int cpu_num)
+{
+ return make_path(cpu_num, false);
+}
+
+static const char *make_output_path(int cpu_num)
+{
+ return make_path(cpu_num, true);
+}
+
+static void *agent_info_init(struct agent_info *s)
+{
+ int cpu;
+ const char *in_path = NULL;
+ const char *out_path = NULL;
+
+ /* init read/write threads */
+ for (cpu = 0; cpu < s->cpus; cpu++) {
+ /* set read(input) path per read/write thread */
+ in_path = make_input_path(cpu);
+ if (in_path == NULL)
+ goto error;
+
+ /* set write(output) path per read/write thread*/
+ if (!s->use_stdout) {
+ out_path = make_output_path(cpu);
+ if (out_path == NULL)
+ goto error;
+ } else
+ /* stdout mode */
+ pr_debug("stdout mode\n");
+
+ rw_thread_init(cpu, in_path, out_path, s->use_stdout,
+ s->pipe_size, s->rw_ti[cpu]);
+ }
+
+ /* init controller of read/write threads */
+ s->ctl_fd = rw_ctl_init((const char *)CTL_PATH);
+
+ return NULL;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+static void *parse_args(int argc, char *argv[], struct agent_info *s)
+{
+ int cmd;
+ unsigned long size;
+
+ while ((cmd = getopt(argc, argv, "hos:")) != -1) {
+ switch (cmd) {
+ /* stdout mode */
+ case 'o':
+ s->use_stdout = true;
+ break;
+ /* size of pipe */
+ case 's':
+ size = parse_size(optarg);
+ if (size == 0)
+ goto error;
+ s->pipe_size = size;
+ break;
+ case 'h':
+ default:
+ usage(argv[0]);
+ goto error;
+ }
+ }
+
+ agent_info_init(s);
+
+ return NULL;
+
+error:
+ exit(EXIT_FAILURE);
+}
+
+static void agent_main_loop(struct agent_info *s)
+{
+ int cpu;
+ pthread_t rw_thread_per_cpu[MAX_CPUS];
+
+ /* Start all read/write threads */
+ for (cpu = 0; cpu < s->cpus; cpu++)
+ rw_thread_per_cpu[cpu] = rw_thread_run(s->rw_ti[cpu]);
+
+ rw_ctl_loop(s->ctl_fd);
+
+ /* Finish all read/write threads */
+ for (cpu = 0; cpu < s->cpus; cpu++) {
+ int ret;
+
+ ret = pthread_join(rw_thread_per_cpu[cpu], NULL);
+ if (ret != 0) {
+ pr_err("pthread_join() error:%d (cpu %d)\n", ret, cpu);
+ exit(EXIT_FAILURE);
+ }
+ }
+}
+
+static void agent_info_free(struct agent_info *s)
+{
+ int i;
+
+ close(s->ctl_fd);
+ for (i = 0; i < s->cpus; i++) {
+ close(s->rw_ti[i]->in_fd);
+ close(s->rw_ti[i]->out_fd);
+ close(s->rw_ti[i]->read_pipe);
+ close(s->rw_ti[i]->write_pipe);
+ free(s->rw_ti[i]);
+ }
+ free(s);
+}
+
+int main(int argc, char *argv[])
+{
+ struct agent_info *s = NULL;
+
+ s = agent_info_new();
+ parse_args(argc, argv, s);
+
+ agent_main_loop(s);
+
+ agent_info_free(s);
+
+ return 0;
+}
diff --git a/tools/virtio/virtio-trace/trace-agent.h b/tools/virtio/virtio-trace/trace-agent.h
new file mode 100644
index 0000000..4711a75
--- /dev/null
+++ b/tools/virtio/virtio-trace/trace-agent.h
@@ -0,0 +1,75 @@
+#ifndef __GUEST_AGENT_H__
+#define __GUEST_AGENT_H__
+#include <pthread.h>
+#include <stdbool.h>
+
+#define MAX_CPUS 256
+#define PIPE_INIT (1024*1024)
+
+/*
+ * agent_info - structure managing total information of guest agent
+ * @pipe_size: size of pipe (default 1MB)
+ * @use_stdout: set to true when o option is added (default false)
+ * @cpus: total number of CPUs
+ * @ctl_fd: fd of control path, /dev/virtio-ports/agent-ctl-path
+ * @rw_ti: structure managing information of read/write threads
+ */
+struct agent_info {
+ unsigned long pipe_size;
+ bool use_stdout;
+ int cpus;
+ int ctl_fd;
+ struct rw_thread_info *rw_ti[MAX_CPUS];
+};
+
+/*
+ * rw_thread_info - structure managing a read/write thread a cpu
+ * @cpu_num: cpu number operating this read/write thread
+ * @in_fd: fd of reading trace data path in cpu_num
+ * @out_fd: fd of writing trace data path in cpu_num
+ * @read_pipe: fd of read pipe
+ * @write_pipe: fd of write pipe
+ * @pipe_size: size of pipe (default 1MB)
+ */
+struct rw_thread_info {
+ int cpu_num;
+ int in_fd;
+ int out_fd;
+ int read_pipe;
+ int write_pipe;
+ unsigned long pipe_size;
+};
+
+/* use for stopping rw threads */
+extern bool global_sig_receive;
+
+/* use for notification */
+extern bool global_run_operation;
+extern pthread_mutex_t mutex_notify;
+extern pthread_cond_t cond_wakeup;
+
+/* for controller of read/write threads */
+extern int rw_ctl_init(const char *ctl_path);
+extern void *rw_ctl_loop(int ctl_fd);
+
+/* for trace read/write thread */
+extern void *rw_thread_info_new(void);
+extern void *rw_thread_init(int cpu, const char *in_path, const char *out_path,
+ bool stdout_flag, unsigned long pipe_size,
+ struct rw_thread_info *rw_ti);
+extern pthread_t rw_thread_run(struct rw_thread_info *rw_ti);
+
+static inline void *zalloc(size_t size)
+{
+ return calloc(1, size);
+}
+
+#define pr_err(format, ...) fprintf(stderr, format, ## __VA_ARGS__)
+#define pr_info(format, ...) fprintf(stdout, format, ## __VA_ARGS__)
+#ifdef DEBUG
+#define pr_debug(format, ...) fprintf(stderr, format, ## __VA_ARGS__)
+#else
+#define pr_debug(format, ...) do {} while (0)
+#endif
+
+#endif /*__GUEST_AGENT_H__*/
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox