netdev - Re: [PATCH 3/3] ifb: move tq from ifb

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 4 Dec 2010 21:29:23 +0800
From:	Changli Gao <xiaosuo@...il.com>
To:	Jarek Poplawski <jarkao2@...il.com>
Cc:	jamal <hadi@...erus.ca>, netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] ifb: move tq from ifb_private

On Sat, Dec 4, 2010 at 9:15 PM, Jarek Poplawski <jarkao2@...il.com> wrote:
> Changli Gao wrote:
>> tq is only used in ri_tasklet, so we move it from ifb_private to a
>> stack variable of ri_tasklet.
>>
>> skb_queue_splice_tail_init() is used instead of the open coded and slow
>> one.
>>
>> Signed-off-by: Changli Gao <xiaosuo@...il.com>
>> ---
>>  drivers/net/ifb.c |   49 ++++++++++++-------------------------------------
>>  1 file changed, 12 insertions(+), 37 deletions(-)
>> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
>> index d1e362a..cd6e90d 100644
>> --- a/drivers/net/ifb.c
>> +++ b/drivers/net/ifb.c
>> @@ -39,9 +39,7 @@
>>  #define TX_Q_LIMIT    32
>>  struct ifb_private {
>>       struct tasklet_struct   ifb_tasklet;
>> -     int     tasklet_pending;
>>       struct sk_buff_head     rq;
>> -     struct sk_buff_head     tq;
>>  };
>>
>>  static int numifbs = 2;
>> @@ -53,27 +51,25 @@ static int ifb_close(struct net_device *dev);
>>
>>  static void ri_tasklet(unsigned long dev)
>>  {
>> -
>>       struct net_device *_dev = (struct net_device *)dev;
>>       struct ifb_private *dp = netdev_priv(_dev);
>>       struct net_device_stats *stats = &_dev->stats;
>>       struct netdev_queue *txq;
>>       struct sk_buff *skb;
>> +     struct sk_buff_head tq;
>>
>> +     __skb_queue_head_init(&tq);
>>       txq = netdev_get_tx_queue(_dev, 0);
>> -     if ((skb = skb_peek(&dp->tq)) == NULL) {
>> -             if (__netif_tx_trylock(txq)) {
>> -                     while ((skb = skb_dequeue(&dp->rq)) != NULL) {
>> -                             skb_queue_tail(&dp->tq, skb);
>> -                     }
>> -                     __netif_tx_unlock(txq);
>> -             } else {
>> -                     /* reschedule */
>> -                     goto resched;
>> -             }
>> +     if (!__netif_tx_trylock(txq)) {
>> +             tasklet_schedule(&dp->ifb_tasklet);
>> +             return;
>>       }
>> +     skb_queue_splice_tail_init(&dp->rq, &tq);
>> +     if (netif_tx_queue_stopped(txq))
>> +             netif_tx_wake_queue(txq);
>> +     __netif_tx_unlock(txq);
>>
>> -     while ((skb = skb_dequeue(&dp->tq)) != NULL) {
>> +     while ((skb = __skb_dequeue(&tq)) != NULL) {
>>               u32 from = G_TC_FROM(skb->tc_verd);
>>
>>               skb->tc_verd = 0;
>> @@ -87,7 +83,7 @@ static void ri_tasklet(unsigned long dev)
>>                       rcu_read_unlock();
>>                       dev_kfree_skb(skb);
>>                       stats->tx_dropped++;
>> -                     break;
>> +                     continue;
>
> IMHO this line is a bugfix and should be a separate patch (for stable).

It sounds reasonable.

>
> The rest looks OK to me but you could probably skip locking of dp->rq
> similarly to tq.
>

OK. Thanks.


-- 
Regards,
Changli Gao(xiaosuo@...il.com)
--
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