From: Stephen Hemminger <stephen@networkplumber.org>
To: Dexuan Cui <decui@microsoft.com>
Cc: kimbrownkd <kimbrownkd@gmail.com>,
Michael Kelley <mikelley@microsoft.com>,
Long Li <longli@microsoft.com>,
Sasha Levin <Alexander.Levin@microsoft.com>,
"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
Haiyang Zhang <haiyangz@microsoft.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] Drivers: hv: vmbus: Expose counters for interrupts and full conditions
Date: Thu, 17 Jan 2019 09:11:03 -0800 [thread overview]
Message-ID: <20190117091019.1fb0c186@hermes.lan> (raw)
In-Reply-To: <KU1P153MB0166DA6E76D0080C4AD43E01BF830@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM>
> +static ssize_t channel_intr_in_full_show(const struct vmbus_channel
> *channel,
> + char *buf)
> +{
> + return sprintf(buf, "%llu\n", channel->intr_in_full);
> +}
intr_in_full is u64, which is not the same as unsigned long long.
to be correct you need a cast here.
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index dcb6977afce9..7e5239123276 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -751,6 +751,27 @@ struct vmbus_channel {
> > u64 interrupts; /* Host to Guest interrupts */
> > u64 sig_events; /* Guest to Host events */
> >
> > + /* Interrupt counts for 2 types of Guest to Host interrupts */
> > + u64 intr_in_full; /* in ring buffer, full to not full */
> > + u64 intr_out_empty; /* out ring buffer, empty to not empty */
> > +
> > + /*
> > + * The total number of write operations that encountered a full
> > + * outbound ring buffer.
> > + */
> > + u64 out_full_total;
> > + /*
> > + * The number of write operations that were the first to encounter a
> > + * full outbound ring buffer.
> > + */
> > + u64 out_full_first;
Adding more fields changes cache layout which can cause
additional cache miss in the hot path.
> > + /*
> > + * Indicates that a full outbound ring buffer was encountered. The flag
> > + * is set to true when a full outbound ring buffer is encountered and
> > + * set to false when a write to the outbound ring buffer is completed.
> > + */
> > + bool out_full_flag;
Discussion on kernel mailing list. Recommends against putting bool
in structures since that pads to full sizeof(int). Could this be
part of a bitfield?
> > /* Channel callback's invoked in softirq context */
> > struct tasklet_struct callback_event;
> > void (*onchannel_callback)(void *context);
> > @@ -936,6 +957,23 @@ static inline void *get_per_channel_state(struct
> > vmbus_channel *c)
> > static inline void set_channel_pending_send_size(struct vmbus_channel *c,
> > u32 size)
> > {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->outbound.ring_lock, flags);
> > +
> > + if (size) {
> > + ++c->out_full_total;
> > +
> > + if (!c->out_full_flag) {
> > + ++c->out_full_first;
> > + c->out_full_flag = true;
> > + }
> > + } else {
> > + c->out_full_flag = false;
> > + }
> > +
> > + spin_unlock_irqrestore(&c->outbound.ring_lock, flags);
If this is called often, the additional locking will impact performance.
> > c->outbound.ring_buffer->pending_send_sz = size;
> > }
> >
Could I propose another alternative.
It might be more useful to count the guest to host interaction events
rather than the ring buffer.
For example the number of calls to:
vmbus_set_event which means host exit call
vmbus_setevent fastpath using sync_set_bit
calls to rinbuffer_write that returned -EAGAIN
These would require less locking, reuse existing code paths
and not require additional state.
next prev parent reply other threads:[~2019-01-17 17:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <0181213190901.GA3843@ubu-Virtual-Machine>
2019-01-05 4:35 ` [PATCH v2] Drivers: hv: vmbus: Expose counters for interrupts and full conditions Kimberly Brown
2019-01-05 21:01 ` Michael Kelley
2019-01-10 3:58 ` Dexuan Cui
2019-01-10 3:46 ` Dexuan Cui
2019-01-17 4:37 ` [PATCH v3] " Kimberly Brown
2019-01-17 6:45 ` Dexuan Cui
2019-01-17 17:11 ` Stephen Hemminger [this message]
2019-01-21 16:29 ` Kimberly Brown
2019-01-17 16:04 ` Michael Kelley
2019-02-04 7:13 ` [PATCH v4] " Kimberly Brown
2019-02-04 16:25 ` Michael Kelley
2019-02-15 1:57 ` Sasha Levin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190117091019.1fb0c186@hermes.lan \
--to=stephen@networkplumber.org \
--cc=Alexander.Levin@microsoft.com \
--cc=decui@microsoft.com \
--cc=devel@linuxdriverproject.org \
--cc=haiyangz@microsoft.com \
--cc=kimbrownkd@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=longli@microsoft.com \
--cc=mikelley@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).