From: Dexuan Cui <decui@microsoft.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
CC: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "stephen@networkplumber.org" <stephen@networkplumber.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "driverdev-devel@linuxdriverproject.org" 
	<driverdev-devel@linuxdriverproject.org>,
        "olaf@aepfle.de" <olaf@aepfle.de>,
        "apw@canonical.com" <apw@canonical.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        KY Srinivasan <kys@microsoft.com>,
        "pebolle@tiscali.nl" <pebolle@tiscali.nl>,
        "stefanha@redhat.com" <stefanha@redhat.com>
Subject: RE: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock
 packet and get the r/w-ability
Thread-Topic: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock
 packet and get the r/w-ability
Thread-Index: AQHQw76VqYGDBjPNEkOE/olvP7hKHJ3nOw2g
Date: Wed, 22 Jul 2015 10:09:10 +0000
Message-ID: <4538264677374d43961c2d709afb1dd3@SIXPR30MB031.064d.mgd.msft.net>
References: <1437476293-6837-1-git-send-email-decui@microsoft.com>
 <87zj2pk3jf.fsf@vitty.brq.redhat.com>
In-Reply-To: <87zj2pk3jf.fsf@vitty.brq.redhat.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
BLUPR03MB1346: X-MS-Exchange-Organization-RulesExecuted
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jul 2015 10:09:16.1549
 (UTC)
X-MS-Exchange-CrossTenant-Id: 72f988bf-86f1-41af-91ab-2d7cd011db47
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=72f988bf-86f1-41af-91ab-2d7cd011db47;Ip=[206.191.229.116];Helo=[064-smtp-out.microsoft.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR03MB1346
X-Microsoft-Exchange-Diagnostics: 1;BLUPR03MB421;2:jg96lVoDAk7Cgg27YuUoxdMhNZUAuWlpDwMyLCo6X81dnpUsmudxqEXNbWUhvYALRccBSp/XHulJ4EIS2UXb+o1QRnUO62JIUZWGHerH/bgpAfs36wmQybMLEMHgTf4lihDwr9or+Mw2Bzd0NC4RhJxTzqDqM8G3wlfzQiA6bf8=;3:3gb9jzLCPT3OqEzs25dyjnmJz+o1hf6P/SX4cIpROO6KP6HfOwli3LjQe3kUtOlc/c6R2GRIB5UCzypeZJiPaCZSBhb1WRsUhazctn+jkFn965xhWXhyXMhN24b7aICWFOCV1G2O1O88OEMQp3QIN8LjeS9d0bquYTnN4LXtCOXS3uOeU3HUUAlSIiHpQO9rYgL1VTTfyrBoR0XljvPrCp9Tm2nVvAOZ39LdzC8gEDZHbMkpwmeOjcMBOy7QdM0C;25:+m0L+JHmrFLo2F8/jRq08W/exqWeTKFtP1i/3KOlDAN4PxiIQzkK+tZgCq/CWJtUuaj9hc0tvQb9OU0SVIvti1fcwbRzEbOJTlFhK/sGEgD/UM8Hng3Tqx2COSKEApKna85GJLR1gLNTngXVK/rzuCtxnxj04hjnsgYID2BhPffh7l7/SxhrmSrrXYdJu2BtRCvBy0ytp0Ce1KUYngHr/geFoSMnrCgg6tcZYEylUqaaT3QOS9noVXyruwavGc4S05G6xsXJL6VkMipq+w2vPQ==;20:t4qwggs+i86TjGCF9ZzjLX/3LYo0l+IlmY6S2GhFUh8t62PzM9bDEYE3s7xM5zYn6mjSdSusFs9xGGo/x+yRyg==
BLUPR03MB421: X-MS-Exchange-Organization-RulesExecuted
X-Microsoft-Exchange-Diagnostics: 1;BLUPR03MB421;23:uF4kX/EQEkOZWeHlPt+qY1+8cT1dM2JO3Kz1hTuS8kk1MWlRpa9XZqR6h1Tre9wjimFlr9zcCqeEI3XQR73IDL5H4jRQW2wkHgte6CsfcPVdaf9+aoL5Zbn/G2ZZmpC4cKwFpQF4K1xUHbo6mRBC/g4482/Fg6Jc0W4ap0XVk5p84S8NQ7PJCqp1aMmU5TJE/O76fFj1VhBlErYSslT+SD60OZr4msWi4xuiY0Hzhf/TPghUGEKiPlBkKi2UC9i0
X-OriginatorOrg: microsoft.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4438
Lines: 131

> From: Vitaly Kuznetsov
> Sent: Tuesday, July 21, 2015 22:07
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> ...
> > +int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void *buffer,
> > +			    u32 bufferlen, u32 *buffer_actual_len)
> > +{
> > +	struct vmpipe_proto_header *pipe_hdr;
> > +	struct vmpacket_descriptor *desc;
> > +	u32 packet_len, payload_len;
> > +	bool signal = false;
> > +	int ret;
> > +
> > +	*buffer_actual_len = 0;
> > +
> > +	if (bufferlen < HVSOCK_HEADER_LEN)
> > +		return -ENOBUFS;
> > +
> > +	ret = hv_ringbuffer_peek(&channel->inbound, buffer,
> > +				 HVSOCK_HEADER_LEN);
> > +	if (ret != 0)
> > +		return 0;
> 
> I'd suggest you do something like
> 
>     if (ret == -EAGIAIN)
>         return 0;
>     else if (ret)
>         return ret;
> 
> to make it future-proof (e.g. when a new error is returned by
> hv_ringbuffer_peek). And a comment would also be useful as it is unclear
> why we silence errors here.
Hi Vitaly,
Thanks! 
I think I made a mistake here:
the "if (ret != 0) return 0;" should be changed
to   "if (ret != 0) return ret;"

vmbus_recvpacket_hvsock() is only invoked in hvsock_recvmsg() in the
case of can_read == true && need_refill == true, which means 
the bytes-available-to-read in the ringbuffer is bigger than
HVSOCK_HEADER_LEN, so here hv_ringbuffer_peek() can only return 0.

I'll fix this in V4.

> > +void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				bool *can_read, bool *can_write)
> > +{
> > +	u32 avl_read_bytes, avl_write_bytes, dummy;
> > +
> > +	if (can_read != NULL) {
> > +		hv_get_ringbuffer_available_space(&channel->inbound,
> > +						  &avl_read_bytes,
> > +						  &dummy);
> > +		*can_read = avl_read_bytes >= HVSOCK_MIN_PKT_LEN;
> > +	}
> > +
> > +	/* We write into the ringbuffer only when we're able to write a
> 
> Not sure why checkpatch.pl doesn't complain but according to the
> CodingStyle multi-line comments outside of drivers/net and net/ are
> supposed to start with and empty '/*' line.
Yeah, you're correct. I'll fix this in V4.

BTW, it seems the rule is not strictly obeyed:  :-)
kernel/sched/fair.c:7290:       /* don't kick the active_load_balance_cpu_stop,
kernel/pid.c:273:                       /* When all that is left in the pid namespace
kernel/watchdog.c:293:  /* check for a hardlockup e
kernel/signal.c:2376:   /* A signal was successfully delivered, and the

> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > @@ -885,6 +885,9 @@ extern int vmbus_sendpacket_ctl(struct
> vmbus_channel *channel,
> >  				  u32 flags,
> >  				  bool kick_q);
> >
> > +extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,
> > +				   void *buf, u32 len);
> > +
> 
> I *think* if your argument list makes it to the second line it is supposed
> to be alligned with the first one (e.g. 'void' should start at the same
> position as 'struct' on the previous line).

I think it's indeed aligned in the patch, i.e., here the 's' and the 'v' are at the
same column.

If I use Vim's ":set list" command, it will like like

extern int vmbus_sendpacket_hvsock(struct vmbus_channel *channel,$
^I^I^I^I   void *buf, u32 len);$

Here I use 4 Tabs and 3 white spaces to make sure the 's' and the 'v' are at
the same column.
In the patch, due to the leading '+', they doesn't look like at the same column.

Please let me know if I'm not using the correct aligning method.
To be frank, I might depend too much on scripts/checkpatch.pl, which didn't
find obvious issues in the patch. :-)

> >  extern int vmbus_sendpacket_pagebuffer(struct vmbus_channel *channel,
> >  					    struct hv_page_buffer pagebuffers[],
> >  					    u32 pagecount,
> > @@ -934,6 +937,11 @@ extern int vmbus_recvpacket_raw(struct
> vmbus_channel *channel,
> >  				     u32 *buffer_actual_len,
> >  				     u64 *requestid);
> >
> > +extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void
> *buffer,
> > +				   u32 bufferlen, u32 *buffer_actual_len);
> > +
> 
> the same.
ditto

> > +extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
> > +				       bool *can_read, bool *can_write);
> >
> 
> the same.
ditto.

Thanks,
-- Dexuan
--
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/