Skip to contentSkip to conversation

FAILED: patch "[PATCH] Drivers: hv: vmbus: finally fix hv_need_to_signal_on_read()" failed to apply to 4.9-stable tree

PATCH [email protected] 1 participant 4 messages Feb 5, 2017 – Feb 11, 2017 2 messages not in the archive

Subsystems (3)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From 433e19cf33d34bb6751c874a9c00980552fe508c Mon Sep 17 00:00:00 2001

From: Dexuan Cui <[email protected]>
Date: Sat, 28 Jan 2017 11:46:02 -0700
Subject: [PATCH] Drivers: hv: vmbus: finally fix hv_need_to_signal_on_read()

Commit a389fcfd2cb5 ("Drivers: hv: vmbus: Fix signaling logic in hv_need_to_signal_on_read()")
added the proper mb(), but removed the test "prev_write_sz < pending_sz" when making the signal decision.

As a result, the guest can signal the host unnecessarily, and then the host can throttle the guest because the host thinks the guest is buggy or malicious; finally the user running stress test can perceive intermittent freeze of the guest.

This patch brings back the test, and properly handles the in-place consumption APIs used by NetVSC (see get_next_pkt_raw(), put_pkt_raw() and commit_rd_index()).

Fixes a389fcfd2cb5 ("Drivers: hv: vmbus: Fix signaling logic in

hv_need_to_signal_on_read()")

drivers/hv/ring_buffer.c +1 -0
index cd49cb17eb7f..308dbda700eb 100644
@@ -383,6 +383,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 		return ret;
 	}
 
+	init_cached_read_index(channel);
 	next_read_location = hv_get_next_read_location(inring_info);
 	next_read_location = hv_copyfrom_ringbuffer(inring_info, &desc,
 						    sizeof(desc),
drivers/net/hyperv/netvsc.c +6 -0
index 5a1cc089acb7..86e5749226ef 100644
@@ -1295,6 +1295,9 @@ void netvsc_channel_cb(void *context)
 	ndev = hv_get_drvdata(device);
 	buffer = get_per_channel_state(channel);
 
+	/* commit_rd_index() -> hv_signal_on_read() needs this. */
+	init_cached_read_index(channel);
+
 	do {
 		desc = get_next_pkt_raw(channel);
 		if (desc != NULL) {
@@ -1347,6 +1350,9 @@ void netvsc_channel_cb(void *context)
 
 			bufferlen = bytes_recvd;
 		}
+
+		init_cached_read_index(channel);
+
 	} while (1);
 
 	if (bufferlen > NETVSC_PACKET_SIZE)
include/linux/hyperv.h +30 -2
index 42fe43fb0c80..183efde54269 100644
@@ -128,6 +128,7 @@ struct hv_ring_buffer_info {
 	u32 ring_data_startoffset;
 	u32 priv_write_index;
 	u32 priv_read_index;
+	u32 cached_read_index;
 };
 
 /*
@@ -180,6 +181,19 @@ static inline u32 hv_get_bytes_to_write(struct hv_ring_buffer_info *rbi)
 	return write;
 }
 
+static inline u32 hv_get_cached_bytes_to_write(
+	const struct hv_ring_buffer_info *rbi)
+{
+	u32 read_loc, write_loc, dsize, write;
+
+	dsize = rbi->ring_datasize;
+	read_loc = rbi->cached_read_index;
+	write_loc = rbi->ring_buffer->write_index;
+
+	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
+		read_loc - write_loc;
+	return write;
+}
 /*
  * VMBUS version is 32 bit entity broken up into
  * two 16 bit quantities: major_number. minor_number.
@@ -1488,7 +1502,7 @@ hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
 
 static inline  void hv_signal_on_read(struct vmbus_channel *channel)
 {
-	u32 cur_write_sz;
+	u32 cur_write_sz, cached_write_sz;
 	u32 pending_sz;
 	struct hv_ring_buffer_info *rbi = &channel->inbound;
 
@@ -1512,12 +1526,24 @@ static inline  void hv_signal_on_read(struct vmbus_channel *channel)
 
 	cur_write_sz = hv_get_bytes_to_write(rbi);
 
-	if (cur_write_sz >= pending_sz)
+	if (cur_write_sz < pending_sz)
+		return;
+
+	cached_write_sz = hv_get_cached_bytes_to_write(rbi);
+	if (cached_write_sz < pending_sz)
 		vmbus_setevent(channel);
 
 	return;
 }
 
+static inline void
+init_cached_read_index(struct vmbus_channel *channel)
+{
+	struct hv_ring_buffer_info *rbi = &channel->inbound;
+
+	rbi->cached_read_index = rbi->ring_buffer->read_index;
+}
+
 /*
  * An API to support in-place processing of incoming VMBUS packets.
  */
@@ -1569,6 +1595,8 @@ static inline void put_pkt_raw(struct vmbus_channel *channel,
  * This call commits the read index and potentially signals the host.
  * Here is the pattern for using the "in-place" consumption APIs:
  *
+ * init_cached_read_index();
+ *
  * while (get_next_pkt_raw() {
  *	process the packet "in-place";
  *	put_pkt_raw();
author maintainer
#2 lore raw
In reply to a message not in the archive
Quoted 8 lines · Rolf Neugebauer

For this patch to apply, we also cherry-pick:
085b322dfe10 ("Drivers: hv: vmbus: Base host signaling strictly on the ring state")
78777547745d ("Drivers: hv: vmbus: On write cleanup the logic to interrupt the host")
983afbbd6d06 ("Drivers: hv: vmbus: On the read path cleanup the logic to interrupt the host")

Quoted 4 lines

With these patches cherry-picked the patch applies cleanly and fixes the original issue we have reported. Our kernel 4.9.8 tree with patches applied is at
https://github.com/rneugeba/linux-stable/tree/v4.9.8-moby

That's a lot of changes. KY and Stephen, is that ok for me to take all of these?

thanks,

greg k-h

author maintainer
#3 lore raw
In reply to a message not in the archive
Quoted 7 lines · Rolf Neugebauer

For this patch to apply, we also cherry-pick:
085b322dfe10 ("Drivers: hv: vmbus: Base host signaling strictly on the ring state")
78777547745d ("Drivers: hv: vmbus: On write cleanup the logic to interrupt the host")
983afbbd6d06 ("Drivers: hv: vmbus: On the read path cleanup the logic to interrupt the host")

Where did you get those commit ids from? I don't see them in Linus's tree anywhere?

And what order do they need to be applied in?

thanks,

greg k-h

author maintainer
#5 lore raw
In reply to a message not in the archive
Quoted 17 lines · Rolf Neugebauer
Quoted 9 lines · Greg KH
Quoted 7 lines · Rolf Neugebauer

For this patch to apply, we also cherry-pick:
085b322dfe10 ("Drivers: hv: vmbus: Base host signaling strictly on the ring state")
78777547745d ("Drivers: hv: vmbus: On write cleanup the logic to interrupt the host")
983afbbd6d06 ("Drivers: hv: vmbus: On the read path cleanup the logic to interrupt the host")

Quoted 4 lines

Where did you get those commit ids from? I don't see them in Linus's tree anywhere?

And what order do they need to be applied in?

Sorry, should have mentioned that I picked them from linux-next.

Quoted 7 lines

In Linus's tree they are

1: 74198eb4a42c4a3c4fbef08fa01a291a282f7c2e
2: 1f6ee4e7d83586c8b10bd4f2f4346353d04ce884
3: 3372592a140db69fd63837e81f048ab4abf8111e

Applied in the order indicated.

Thanks, that worked!

greg k-h