Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts()
From: Jesse Brandeburg <hidden>
Date: 2018-05-21 16:39:08
On Mon, 21 May 2018 17:04:24 +0800 Jason wrote:
quoted hunk ↗ jump to hunk
Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/net.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index de544ee..4ebac76 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c@@ -485,6 +485,13 @@ static bool vhost_exceeds_weight(int pkts, int total_len) unlikely(pkts >= VHOST_NET_PKT_WEIGHT); } +static bool vhost_has_more_pkts(struct vhost_net *net, + struct vhost_virtqueue *vq) +{ + return !vhost_vq_avail_empty(&net->dev, vq) && + likely(!vhost_exceeds_maxpend(net));
This really seems like mis-use of likely/unlikely, in the middle of a sequence of operations that will always be run when this function is called. I think you should remove the likely from this helper, especially, and control the branch from the branch point.
quoted hunk ↗ jump to hunk
+} + /* Expects to be always run from workqueue - which acts as * read-size critical section for our kind of RCU. */ static void handle_tx(struct vhost_net *net)@@ -578,8 +585,7 @@ static void handle_tx(struct vhost_net *net) } total_len += len; if (total_len < VHOST_NET_WEIGHT && - !vhost_vq_avail_empty(&net->dev, vq) && - likely(!vhost_exceeds_maxpend(net))) { + vhost_has_more_pkts(net, vq)) {
Yes, I know it came from here, but likely/unlikely are for branch control, so they should encapsulate everything inside the if, unless I'm mistaken.
quoted hunk ↗ jump to hunk
msg.msg_flags |= MSG_MORE; } else { msg.msg_flags &= ~MSG_MORE;@@ -605,7 +611,7 @@ static void handle_tx(struct vhost_net *net) else vhost_zerocopy_signal_used(net, vq); vhost_net_tx_packet(net); - if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) { + if (vhost_exceeds_weight(++sent_pkts, total_len)) {
You should have kept the unlikely here, and not had it inside the helper (as per the previous patch. Also, why wasn't this change part of the previous patch?
vhost_poll_queue(&vq->poll); break; }