| Message ID | 1526893473-20128-4-git-send-email-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +} > + > /* 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. > 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; > }
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; >> }
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)); +} + /* 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)) { 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)) { vhost_poll_queue(&vq->poll); break; }
Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/vhost/net.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)