Re: [PATCH V3 3/7] Drivers: hv: vmbus: add APIs to send/recv hvsock packet and get the r/w-ability

Dexuan Cui [off-list ref] writes:

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.

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.

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

the same.

+extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				       bool *can_read, bool *can_write);

the same.

-- 
  Vitaly

From: Vitaly Kuznetsov
Sent: Tuesday, July 21, 2015 22:07

quoted

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.

quoted

+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

quoted

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,

quoted

 				  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. :-)

quoted

 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,

quoted

 				     u32 *buffer_actual_len,
 				     u64 *requestid);

+extern int vmbus_recvpacket_hvsock(struct vmbus_channel *channel, void

*buffer,

quoted

+				   u32 bufferlen, u32 *buffer_actual_len);
+

the same.

ditto

quoted

+extern void vmbus_get_hvsock_rw_status(struct vmbus_channel *channel,
+				       bool *can_read, bool *can_write);

the same.

ditto.

Thanks,
-- Dexuan

On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:

quoted

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;"

The double negative really doesn't not make the code more complicated.
I like using a quadruple negative instead.

	if (ret != 0 != 0)
		return ret;

regards,
dan carpenter

From: Dan Carpenter
Sent: Wednesday, July 22, 2015 18:36
To: Dexuan Cui
On Wed, Jul 22, 2015 at 10:09:10AM +0000, Dexuan Cui wrote:

quoted

quoted

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;"

Usually 0 means success to me, so 
to me, "ret != 0" reads like "ret is not successful" and seems natural. 

The kind of usage is not rare in the kernel code:

decui@lin:~/linux-next$ grep 'if (ret != 0)' kernel/ include/ ipc/  -r  | wc -l
28
decui@lin:~/linux-next$ grep 'if (ret != 0)' drivers/   -r  | wc -l
1031
 

The double negative really doesn't not make the code more complicated.
I like using a quadruple negative instead.

	if (ret != 0 != 0)
		return ret;
dan carpenter

Hi Dan, I read this as a humor.  :-)

I'll take the suggestion and remember to use this in V4 and in future:
 
if (ret)
	return ret;

Thanks!

-- Dexuan

On Thu, Jul 23, 2015 at 03:05:16AM +0000, Dexuan Cui wrote:

The kind of usage is not rare in the kernel code:

Yeah.  But it's used 5% of the time.  If it's under 15% then there is a
risk that we'll write a checkpatch rule to enforce the standard way...
There are some places where != 0 is idiomatic, like when you are talking
about the number zero.  strcmp() and friends should always be != 0 or
== 0.

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

Hi Dan, I read this as a humor.  :-)

:)

regards,
dan carpenter

On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
drivers/regulator/wm8400-regulator.c:47         return 0;
drivers/platform/x86/dell-laptop.c:859          return 0;
drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
drivers/hv/channel.c:898                return 0;
drivers/hv/channel.c:944                return 0;

A bunch of those look suspicious but I don't know the subsystems well
enough to be sure.  Can you check the last two?

regards,
dan carpenter

From: Dan Carpenter
Sent: Thursday, July 23, 2015 18:25
On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:

quoted

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/;
print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;
drivers/regulator/wm8400-regulator.c:47         return 0;
drivers/platform/x86/dell-laptop.c:859          return 0;
drivers/media/dvb-frontends/dibx000_common.c:213                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:217                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:235                                return 0;
drivers/media/dvb-frontends/dibx000_common.c:239                                return 0;
drivers/hv/channel.c:898                return 0;
drivers/hv/channel.c:944                return 0;

A bunch of those look suspicious but I don't know the subsystems well
enough to be sure.  Can you check the last two?

dan carpenter

Thanks, Dan!

After I checked the code, I think there is no issue for the last two:
in the case of "if (ret != 0) return 0;",  the output parameter buffer_actual_len is
zero and it is explicitly checked by the callers.
This may seem not natural and I think we can improve it in future.

BTW, at the end of vmbus_recvpacket(), the "return 0;" should be "return ret;", but
since the output parameter buffer_actual_len is checked by the callers, I think it's
OK for now.

Thanks,
-- Dexuan

On Thu, Jul 23, 2015 at 01:24:50PM +0300, Dan Carpenter wrote:

On Thu, Jul 23, 2015 at 01:10:57PM +0300, Dan Carpenter wrote:

quoted

In this specific case, writing it as "if (ret != 0)" caused the bug.  If
we had written it as "if (ret) return ret;" then there are no zeroes so
wouldn't have been any temptation to return the zero instead of the ret.

I did a search to see if returning the zero instead of the ret was a
common mistake and it seems like it might be.  I did:

grep 'if (ret != 0)' drivers/   -r -A1 -n | grep "return 0;" | perl -ne 's/.c-(\d+)-/.c:$1/; print'

drivers/gpu/drm/nouveau/nouveau_display.c:111                   return 0;

This is also ok, the function is supposed to return ret or-ed with the
relevant flags based on the scan position. It is considered error if 0
is returned (without any flag).

regards
sudip

On Fri, Jul 24, 2015 at 11:57:01AM +0530, Sudip Mukherjee wrote:

This is also ok, the function is supposed to return ret or-ed with the
relevant flags based on the scan position. It is considered error if 0
is returned (without any flag).

Yeah.  You're right.  I looked through my list again this morning and
they all seem fine...

regards,
dan carpenter