by Qin Chuanyu

[permalink] [raw]

Subject: Re: [PATCH RFC v4 net-next 1/5] virtio_net: enable tx interrupt

On 2014/12/1 18:17, Jason Wang wrote:
> On newer hosts that support delayed tx interrupts,
> we probably don't have much to gain from orphaning
> packets early.
>
> Note: this might degrade performance for
> hosts without event idx support.
> Should be addressed by the next patch.
>
> Cc: Rusty Russell <[email protected]>
> Cc: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> drivers/net/virtio_net.c | 132 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 88 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec2a8b4..f68114e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> {
> struct skb_vnet_hdr *hdr;
> @@ -912,7 +951,9 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> sg_set_buf(sq->sg, hdr, hdr_len);
> num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
> }
> - return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
> +
> + return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb,
> + GFP_ATOMIC);
> }
>
> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -924,8 +965,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
> bool kick = !skb->xmit_more;
>
> - /* Free up any pending old buffers before queueing new ones. */
> - free_old_xmit_skbs(sq);

I think there is no need to remove free_old_xmit_skbs here.
you could add free_old_xmit_skbs in tx_irq's napi func.
also could do this in start_xmit if you handle the race well.

I have done the same thing in ixgbe driver(free skb in ndo_start_xmit
and both in tx_irq's poll func), and it seems work well:)

I think there would be no so much interrupts in this way, also tx
interrupt coalesce is not needed.

> + virtqueue_disable_cb(sq->vq);
>
> /* Try to transmit */
> err = xmit_skb(sq, skb);
> @@ -941,27 +981,19 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> return NETDEV_TX_OK;
> }
>
> - /* Don't wait up for transmitted skbs to be freed. */
> - skb_orphan(skb);
> - nf_reset(skb);
> -
> /* Apparently nice girls don't return TX_BUSY; stop the queue
> * before it gets out of hand. Naturally, this wastes entries. */
> - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> + if (sq->vq->num_free < 2+MAX_SKB_FRAGS)
> netif_stop_subqueue(dev, qnum);
> - if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> - /* More just got used, free them then recheck. */
> - free_old_xmit_skbs(sq);
> - if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> - netif_start_subqueue(dev, qnum);
> - virtqueue_disable_cb(sq->vq);
> - }
> - }
> - }
>
> if (kick || netif_xmit_stopped(txq))
> virtqueue_kick(sq->vq);
>
> + if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> + virtqueue_disable_cb(sq->vq);
> + napi_schedule(&sq->napi);
> + }
> +
> return NETDEV_TX_OK;
> }
>
> @@ -1138,8 +1170,10 @@ static int virtnet_close(struct net_device *dev)
> /* Make sure refill_work doesn't re-enable napi! */
> cancel_delayed_work_sync(&vi->refill);
>
> - for (i = 0; i < vi->max_queue_pairs; i++)
> + for (i = 0; i < vi->max_queue_pairs; i++) {
> napi_disable(&vi->rq[i].napi);
> + napi_disable(&vi->sq[i].napi);
> + }
>
> return 0;
> }
> @@ -1452,8 +1486,10 @@ static void virtnet_free_queues(struct virtnet_info *vi)
> {
> int i;
>
> - for (i = 0; i < vi->max_queue_pairs; i++)
> + for (i = 0; i < vi->max_queue_pairs; i++) {
> netif_napi_del(&vi->rq[i].napi);
> + netif_napi_del(&vi->sq[i].napi);
> + }
>
> kfree(vi->rq);
> kfree(vi->sq);

2014-12-19 10:02:30

by Jason Wang

[permalink] [raw]

Subject: Re: [PATCH RFC v4 net-next 1/5] virtio_net: enable tx interrupt

On Fri, Dec 19, 2014 at 3:32 PM, Qin Chuanyu <[email protected]>
wrote:
> On 2014/12/1 18:17, Jason Wang wrote:
>> On newer hosts that support delayed tx interrupts,
>> we probably don't have much to gain from orphaning
>> packets early.
>>
>> Note: this might degrade performance for
>> hosts without event idx support.
>> Should be addressed by the next patch.
>>
>> Cc: Rusty Russell <[email protected]>
>> Cc: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Michael S. Tsirkin <[email protected]>
>> Signed-off-by: Jason Wang <[email protected]>
>> ---
>> drivers/net/virtio_net.c | 132
>> +++++++++++++++++++++++++++++++----------------
>> 1 file changed, 88 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index ec2a8b4..f68114e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
>> {
>> struct skb_vnet_hdr *hdr;
>> @@ -912,7 +951,9 @@ static int xmit_skb(struct send_queue *sq,
>> struct sk_buff *skb)
>> sg_set_buf(sq->sg, hdr, hdr_len);
>> num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
>> }
>> - return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb,
>> GFP_ATOMIC);
>> +
>> + return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb,
>> + GFP_ATOMIC);
>> }
>>
>> static netdev_tx_t start_xmit(struct sk_buff *skb, struct
>> net_device *dev)
>> @@ -924,8 +965,7 @@ static netdev_tx_t start_xmit(struct sk_buff
>> *skb, struct net_device *dev)
>> struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
>> bool kick = !skb->xmit_more;
>>
>> - /* Free up any pending old buffers before queueing new ones. */
>> - free_old_xmit_skbs(sq);
>
> I think there is no need to remove free_old_xmit_skbs here.
> you could add free_old_xmit_skbs in tx_irq's napi func.
> also could do this in start_xmit if you handle the race well.

Note, free_old_xmit_skbs() has already called in tx napi.
It was a must after tx interrupt was enabled.
>
>
> I have done the same thing in ixgbe driver(free skb in ndo_start_xmit
> and both in tx_irq's poll func), and it seems work well:)

Any performance numbers on this change?
I suspect it reduce the effects of interrupt coalescing.
>
> I think there would be no so much interrupts in this way, also tx
> interrupt coalesce is not needed.

Tests (multiple sessions of TCP_RR) does not support this.
Calling free_old_xmit_skbs() in fact damage the performance.

Any justification that you think it may reduce the interrupts?

Thanks
>
>
>> + virtqueue_disable_cb(sq->vq);
>>
>> /* Try to transmit */
>> err = xmit_skb(sq, skb);
>> @@ -941,27 +981,19 @@ static netdev_tx_t start_xmit(struct sk_buff
>> *skb, struct net_device *dev)
>> return NETDEV_TX_OK;
>> }
>>
>> - /* Don't wait up for transmitted skbs to be freed. */
>> - skb_orphan(skb);
>> - nf_reset(skb);
>> -
>> /* Apparently nice girls don't return TX_BUSY; stop the queue
>> * before it gets out of hand. Naturally, this wastes entries. */
>> - if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
>> + if (sq->vq->num_free < 2+MAX_SKB_FRAGS)
>> netif_stop_subqueue(dev, qnum);
>> - if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
>> - /* More just got used, free them then recheck. */
>> - free_old_xmit_skbs(sq);
>> - if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
>> - netif_start_subqueue(dev, qnum);
>> - virtqueue_disable_cb(sq->vq);
>> - }
>> - }
>> - }
>>
>> if (kick || netif_xmit_stopped(txq))
>> virtqueue_kick(sq->vq);
>>
>> + if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
>> + virtqueue_disable_cb(sq->vq);
>> + napi_schedule(&sq->napi);
>> + }
>> +
>> return NETDEV_TX_OK;
>> }
>>
>> @@ -1138,8 +1170,10 @@ static int virtnet_close(struct net_device
>> *dev)
>> /* Make sure refill_work doesn't re-enable napi! */
>> cancel_delayed_work_sync(&vi->refill);
>>
>> - for (i = 0; i < vi->max_queue_pairs; i++)
>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>> napi_disable(&vi->rq[i].napi);
>> + napi_disable(&vi->sq[i].napi);
>> + }
>>
>> return 0;
>> }
>> @@ -1452,8 +1486,10 @@ static void virtnet_free_queues(struct
>> virtnet_info *vi)
>> {
>> int i;
>>
>> - for (i = 0; i < vi->max_queue_pairs; i++)
>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>> netif_napi_del(&vi->rq[i].napi);
>> + netif_napi_del(&vi->sq[i].napi);
>> + }
>>
>> kfree(vi->rq);
>> kfree(vi->sq);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html