Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 17 Jan 2019 09:11:03 -0800
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
Message-ID: <20190117091019.1fb0c186@hermes.lan>
In-Reply-To: <KU1P153MB0166DA6E76D0080C4AD43E01BF830@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM>
References: <20190105043518.GA4072@ubu-Virtual-Machine>
        <20190117043759.GA3395@ubu-Virtual-Machine>
        <KU1P153MB0166DA6E76D0080C4AD43E01BF830@KU1P153MB0166.APCP153.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> +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.