| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1418983324.20738.1@smtp.corp.redhat.com>
Date: Fri, 19 Dec 2014 10:10:04 +0008
From: Jason Wang <jasowang@...hat.com>
To: Qin Chuanyu <qinchuanyu@...wei.com>
Cc: mst@...hat.com, virtualization@...ts.linux-foundation.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
davem@...emloft.net, pagupta@...hat.com,
Rusty Russell <rusty@...tcorp.com.au>
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 <qinchuanyu@...wei.com>
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 <rusty@...tcorp.com.au>
>> Cc: Michael S. Tsirkin <mst@...hat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
>> Signed-off-by: Jason Wang <jasowang@...hat.com>
>> ---
>> 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 majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists