Message-ID: <CAEH94LhM5WWLEETv-n0cYs4wv+Hw4pC7-c+b5cKqRxdx2XywNw@mail.gmail.com>
Date:	Tue, 17 Dec 2013 17:13:02 +0800
From:	Zhi Yong Wu <zwu.kernel@...il.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	netdev@...r.kernel.org,
	linux-kernel mlist <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH] net, tun: remove the flow cache

On Tue, Dec 17, 2013 at 4:49 PM, Jason Wang <jasowang@...hat.com> wrote:
> On 12/17/2013 03:26 PM, Zhi Yong Wu wrote:
>> From: Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>
>>
>> The flow cache is an extremely broken concept, and it usually brings up
>> growth issues and DoS attacks, so this patch is trying to remove it from
>> the tuntap driver, and insteadly use a simpler way for its flow control.
>
> NACK.
>
> This single function revert does not make sense to me. Since:
IIRC, the tuntap flow cache is only used to save the mapping of skb
packet <-> queue index. My idea only save the queue index in skb_buff
early when skb buffer is filled, not in flow cache as the current
code. This method is actually more simpler and completely doesn't need
any flow cache.

>
> - You in fact removes the flow steering function in tun. We definitely
> need something like this to unbreak the TCP performance in a multiqueue
I don't think it will downgrade the TCP perf even in mq guest, but my
idea maybe has better TCP perf, because it doesn't have any cache
table lookup, etc.
> guest. Please have a look at the virtio-net driver / virtio sepc for
> more information.
> - The total number of flow caches were limited to 4096, so there's no
> DoS or growth issue.
Can you check why the ipv4 routing cache is removed? maybe i miss
something, if yes, pls correct me. :)
> - The only issue is scalability, but fixing this is not easy. We can
> just use arrays/indirection table like RSS instead of hash buckets, it
> saves some time in linear search but has other issues like collision
> - I've also had a RFC of using aRFS in the past, it also has several
> drawbacks such as busy looping in the networking hotspot.
>
> So in conclusion, we need flow steering in tun, just removing current
> method does not help. The proper way is to expose several different
> methods to user and let user to choose the preferable mechanism like
> packet.
By the way, let us look at what other networking guys think of this,
such as MST, dave, etc. :)

>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>
>> ---
>>  drivers/net/tun.c |  208 +++--------------------------------------------------
>>  1 files changed, 10 insertions(+), 198 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 7c8343a..7c27fdc 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -32,12 +32,15 @@
>>   *
>>   *  Daniel Podlejski <underley@...erley.eu.org>
>>   *    Modifications for 2.3.99-pre5 kernel.
>> + *
>> + *  Zhi Yong Wu <wuzhy@...ux.vnet.ibm.com>
>> + *    Remove the flow cache.
>>   */
>>
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>>  #define DRV_NAME     "tun"
>> -#define DRV_VERSION  "1.6"
>> +#define DRV_VERSION  "1.7"
>>  #define DRV_DESCRIPTION      "Universal TUN/TAP device driver"
>>  #define DRV_COPYRIGHT        "(C) 1999-2004 Max Krasnyansky <maxk@...lcomm.com>"
>>
>> @@ -146,18 +149,6 @@ struct tun_file {
>>       struct tun_struct *detached;
>>  };
>>
>> -struct tun_flow_entry {
>> -     struct hlist_node hash_link;
>> -     struct rcu_head rcu;
>> -     struct tun_struct *tun;
>> -
>> -     u32 rxhash;
>> -     int queue_index;
>> -     unsigned long updated;
>> -};
>> -
>> -#define TUN_NUM_FLOW_ENTRIES 1024
>> -
>>  /* Since the socket were moved to tun_file, to preserve the behavior of persist
>>   * device, socket filter, sndbuf and vnet header size were restore when the
>>   * file were attached to a persist device.
>> @@ -184,163 +175,11 @@ struct tun_struct {
>>       int debug;
>>  #endif
>>       spinlock_t lock;
>> -     struct hlist_head flows[TUN_NUM_FLOW_ENTRIES];
>> -     struct timer_list flow_gc_timer;
>> -     unsigned long ageing_time;
>>       unsigned int numdisabled;
>>       struct list_head disabled;
>>       void *security;
>> -     u32 flow_count;
>>  };
>>
>> -static inline u32 tun_hashfn(u32 rxhash)
>> -{
>> -     return rxhash & 0x3ff;
>> -}
>> -
>> -static struct tun_flow_entry *tun_flow_find(struct hlist_head *head, u32 rxhash)
>> -{
>> -     struct tun_flow_entry *e;
>> -
>> -     hlist_for_each_entry_rcu(e, head, hash_link) {
>> -             if (e->rxhash == rxhash)
>> -                     return e;
>> -     }
>> -     return NULL;
>> -}
>> -
>> -static struct tun_flow_entry *tun_flow_create(struct tun_struct *tun,
>> -                                           struct hlist_head *head,
>> -                                           u32 rxhash, u16 queue_index)
>> -{
>> -     struct tun_flow_entry *e = kmalloc(sizeof(*e), GFP_ATOMIC);
>> -
>> -     if (e) {
>> -             tun_debug(KERN_INFO, tun, "create flow: hash %u index %u\n",
>> -                       rxhash, queue_index);
>> -             e->updated = jiffies;
>> -             e->rxhash = rxhash;
>> -             e->queue_index = queue_index;
>> -             e->tun = tun;
>> -             hlist_add_head_rcu(&e->hash_link, head);
>> -             ++tun->flow_count;
>> -     }
>> -     return e;
>> -}
>> -
>> -static void tun_flow_delete(struct tun_struct *tun, struct tun_flow_entry *e)
>> -{
>> -     tun_debug(KERN_INFO, tun, "delete flow: hash %u index %u\n",
>> -               e->rxhash, e->queue_index);
>> -     hlist_del_rcu(&e->hash_link);
>> -     kfree_rcu(e, rcu);
>> -     --tun->flow_count;
>> -}
>> -
>> -static void tun_flow_flush(struct tun_struct *tun)
>> -{
>> -     int i;
>> -
>> -     spin_lock_bh(&tun->lock);
>> -     for (i = 0; i < TUN_NUM_FLOW_ENTRIES; i++) {
>> -             struct tun_flow_entry *e;
>> -             struct hlist_node *n;
>> -
>> -             hlist_for_each_entry_safe(e, n, &tun->flows[i], hash_link)
>> -                     tun_flow_delete(tun, e);
>> -     }
>> -     spin_unlock_bh(&tun->lock);
>> -}
>> -
>> -static void tun_flow_delete_by_queue(struct tun_struct *tun, u16 queue_index)
>> -{
>> -     int i;
>> -
>> -     spin_lock_bh(&tun->lock);
>> -     for (i = 0; i < TUN_NUM_FLOW_ENTRIES; i++) {
>> -             struct tun_flow_entry *e;
>> -             struct hlist_node *n;
>> -
>> -             hlist_for_each_entry_safe(e, n, &tun->flows[i], hash_link) {
>> -                     if (e->queue_index == queue_index)
>> -                             tun_flow_delete(tun, e);
>> -             }
>> -     }
>> -     spin_unlock_bh(&tun->lock);
>> -}
>> -
>> -static void tun_flow_cleanup(unsigned long data)
>> -{
>> -     struct tun_struct *tun = (struct tun_struct *)data;
>> -     unsigned long delay = tun->ageing_time;
>> -     unsigned long next_timer = jiffies + delay;
>> -     unsigned long count = 0;
>> -     int i;
>> -
>> -     tun_debug(KERN_INFO, tun, "tun_flow_cleanup\n");
>> -
>> -     spin_lock_bh(&tun->lock);
>> -     for (i = 0; i < TUN_NUM_FLOW_ENTRIES; i++) {
>> -             struct tun_flow_entry *e;
>> -             struct hlist_node *n;
>> -
>> -             hlist_for_each_entry_safe(e, n, &tun->flows[i], hash_link) {
>> -                     unsigned long this_timer;
>> -                     count++;
>> -                     this_timer = e->updated + delay;
>> -                     if (time_before_eq(this_timer, jiffies))
>> -                             tun_flow_delete(tun, e);
>> -                     else if (time_before(this_timer, next_timer))
>> -                             next_timer = this_timer;
>> -             }
>> -     }
>> -
>> -     if (count)
>> -             mod_timer(&tun->flow_gc_timer, round_jiffies_up(next_timer));
>> -     spin_unlock_bh(&tun->lock);
>> -}
>> -
>> -static void tun_flow_update(struct tun_struct *tun, u32 rxhash,
>> -                         struct tun_file *tfile)
>> -{
>> -     struct hlist_head *head;
>> -     struct tun_flow_entry *e;
>> -     unsigned long delay = tun->ageing_time;
>> -     u16 queue_index = tfile->queue_index;
>> -
>> -     if (!rxhash)
>> -             return;
>> -     else
>> -             head = &tun->flows[tun_hashfn(rxhash)];
>> -
>> -     rcu_read_lock();
>> -
>> -     /* We may get a very small possibility of OOO during switching, not
>> -      * worth to optimize.*/
>> -     if (tun->numqueues == 1 || tfile->detached)
>> -             goto unlock;
>> -
>> -     e = tun_flow_find(head, rxhash);
>> -     if (likely(e)) {
>> -             /* TODO: keep queueing to old queue until it's empty? */
>> -             e->queue_index = queue_index;
>> -             e->updated = jiffies;
>> -     } else {
>> -             spin_lock_bh(&tun->lock);
>> -             if (!tun_flow_find(head, rxhash) &&
>> -                 tun->flow_count < MAX_TAP_FLOWS)
>> -                     tun_flow_create(tun, head, rxhash, queue_index);
>> -
>> -             if (!timer_pending(&tun->flow_gc_timer))
>> -                     mod_timer(&tun->flow_gc_timer,
>> -                               round_jiffies_up(jiffies + delay));
>> -             spin_unlock_bh(&tun->lock);
>> -     }
>> -
>> -unlock:
>> -     rcu_read_unlock();
>> -}
>> -
>>  /* We try to identify a flow through its rxhash first. The reason that
>>   * we do not check rxq no. is becuase some cards(e.g 82599), chooses
>>   * the rxq based on the txq where the last packet of the flow comes. As
>> @@ -351,7 +190,6 @@ unlock:
>>  static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb)
>>  {
>>       struct tun_struct *tun = netdev_priv(dev);
>> -     struct tun_flow_entry *e;
>>       u32 txq = 0;
>>       u32 numqueues = 0;
>>
>> @@ -360,12 +198,11 @@ static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb)
>>
>>       txq = skb_get_rxhash(skb);
>>       if (txq) {
>> -             e = tun_flow_find(&tun->flows[tun_hashfn(txq)], txq);
>> -             if (e)
>> -                     txq = e->queue_index;
>> -             else
>> +             txq = skb_get_queue_mapping(skb);
>> +             if (!txq) {
>>                       /* use multiply and shift instead of expensive divide */
>>                       txq = ((u64)txq * numqueues) >> 32;
>> +             }
>>       } else if (likely(skb_rx_queue_recorded(skb))) {
>>               txq = skb_get_rx_queue(skb);
>>               while (unlikely(txq >= numqueues))
>> @@ -439,7 +276,6 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>>                       tun_disable_queue(tun, tfile);
>>
>>               synchronize_net();
>> -             tun_flow_delete_by_queue(tun, tun->numqueues + 1);
>>               /* Drop read queue */
>>               tun_queue_purge(tfile);
>>               tun_set_real_num_queues(tun);
>> @@ -858,25 +694,6 @@ static const struct net_device_ops tap_netdev_ops = {
>>  #endif
>>  };
>>
>> -static void tun_flow_init(struct tun_struct *tun)
>> -{
>> -     int i;
>> -
>> -     for (i = 0; i < TUN_NUM_FLOW_ENTRIES; i++)
>> -             INIT_HLIST_HEAD(&tun->flows[i]);
>> -
>> -     tun->ageing_time = TUN_FLOW_EXPIRE;
>> -     setup_timer(&tun->flow_gc_timer, tun_flow_cleanup, (unsigned long)tun);
>> -     mod_timer(&tun->flow_gc_timer,
>> -               round_jiffies_up(jiffies + tun->ageing_time));
>> -}
>> -
>> -static void tun_flow_uninit(struct tun_struct *tun)
>> -{
>> -     del_timer_sync(&tun->flow_gc_timer);
>> -     tun_flow_flush(tun);
>> -}
>> -
>>  /* Initialize net device. */
>>  static void tun_net_init(struct net_device *dev)
>>  {
>> @@ -986,7 +803,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>>       int copylen;
>>       bool zerocopy = false;
>>       int err;
>> -     u32 rxhash;
>>
>>       if (!(tun->flags & TUN_NO_PI)) {
>>               if (len < sizeof(pi))
>> @@ -1146,13 +962,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>>       skb_reset_network_header(skb);
>>       skb_probe_transport_header(skb, 0);
>>
>> -     rxhash = skb_get_rxhash(skb);
>> +     skb_set_queue_mapping(skb, tfile->queue_index);
>>       netif_rx_ni(skb);
>>
>>       tun->dev->stats.rx_packets++;
>>       tun->dev->stats.rx_bytes += len;
>>
>> -     tun_flow_update(tun, rxhash, tfile);
>>       return total_len;
>>  }
>>
>> @@ -1370,7 +1185,6 @@ static void tun_free_netdev(struct net_device *dev)
>>       struct tun_struct *tun = netdev_priv(dev);
>>
>>       BUG_ON(!(list_empty(&tun->disabled)));
>> -     tun_flow_uninit(tun);
>>       security_tun_dev_free_security(tun->security);
>>       free_netdev(dev);
>>  }
>> @@ -1644,7 +1458,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>>                       goto err_free_dev;
>>
>>               tun_net_init(dev);
>> -             tun_flow_init(tun);
>>
>>               dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>>                                  TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX |
>> @@ -1655,7 +1468,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>>               INIT_LIST_HEAD(&tun->disabled);
>>               err = tun_attach(tun, file, false);
>>               if (err < 0)
>> -                     goto err_free_flow;
>> +                     goto err_free_security;
>>
>>               err = register_netdevice(tun->dev);
>>               if (err < 0)
>> @@ -1705,8 +1518,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>>
>>  err_detach:
>>       tun_detach_all(dev);
>> -err_free_flow:
>> -     tun_flow_uninit(tun);
>> +err_free_security:
>>       security_tun_dev_free_security(tun->security);
>>  err_free_dev:
>>       free_netdev(dev);
>



-- 
Regards,

Zhi Yong Wu
--
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