Messages in this thread | | | Subject | Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts() | From | Jason Wang <> | Date | Tue, 22 May 2018 20:31:40 +0800 |
| |
On 2018年05月22日 00:39, Jesse Brandeburg wrote: > On Mon, 21 May 2018 17:04:24 +0800 Jason wrote: >> 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.
Yes, so I'm consider to make it a macro in next version.
> > >> +} >> + >> /* 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.
Ok.
> >> 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?
Yes, will squash the above into previous one.
Thanks
> >> vhost_poll_queue(&vq->poll); >> break; >> }
| |