Re: [RFC PATCH net-next 03/12] vhost_net: introduce vhost_has_more_pkts()

[Jason Wang]

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@xxxxxxxxxx>
> > ---
> >   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;
> >   		}