netdev - Re: [PATCH v2] net: batch skb dequeueing from softnet input_pkt

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:	Tue, 13 Apr 2010 12:19:02 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Changli Gao <xiaosuo@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH v2] net: batch skb dequeueing from softnet
 input_pkt_queue

Le mardi 13 avril 2010 à 17:50 +0800, Changli Gao a écrit :
> On Tue, Apr 13, 2010 at 4:08 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> >
> >        Probably not necessary.
> >
> >> +     volatile bool           flush_processing_queue;
> >
> > Use of 'volatile' is strongly discouraged, I would say, forbidden.
> >
> 
> volatile is used to avoid compiler optimization.

volatile might be used on special macros only, not to guard a variable.
volatile was pre SMP days. We need something better defined these days.

> 
> > Its usually a sign of 'I dont exactly what memory ordering I need, so I
> > throw a volatile just in case'. We live in a world full of RCU, read ,
> > write, full barriers. And these apis are well documented.
> >
> 
> There isn't memory accessing order problem.
> 

Really ? If you persist using this volatile thing, then you'll have to
convince Mr Linus Torvald himself. Prepare your best guns, because in
fact you'll have to convince about one hundred people that know for sure
how volatile is weak.

> >> @@ -2803,6 +2808,7 @@ static void flush_backlog(void *arg)
> >>                       __skb_unlink(skb, &queue->input_pkt_queue);
> >>                       kfree_skb(skb);
> >>               }
> >> +     queue->flush_processing_queue = true;
> >
> >        Probably not necessary
> >
> 
> If flush_backlog() is called when there are still packets in
> processing_queue, there maybe some packets refer to the netdev gone,
> if we remove this line.

We dont need this "processing_queue". Once you remove it, there is no
extra work to perform.

> 
> >>       rps_unlock(queue);
> >>  }
> >>
> >> @@ -3112,14 +3118,23 @@ static int process_backlog(struct napi_struct *napi, int quota)
> >>       struct softnet_data *queue = &__get_cpu_var(softnet_data);
> >>       unsigned long start_time = jiffies;
> >>
> >> +     if (queue->flush_processing_queue) {
> >
> > Really... this is bloat IMHO
> 
> 
> Any better idea?
> 

Yes, I have explained it to you.

> >
> >>
> >
> > I advise to keep it simple.
> >
> > My suggestion would be to limit this patch only to process_backlog().
> >
> > Really if you touch other areas, there is too much risk.
> >
> > Perform sort of skb_queue_splice_tail_init() into a local (stack) queue,
> > but the trick is to not touch input_pkt_queue.qlen, so that we dont slow
> > down enqueue_to_backlog().
> >
> > Process at most 'quota' skbs (or jiffies limit).
> >
> > relock queue.
> > input_pkt_queue.qlen -= number_of_handled_skbs;
> >
> 
> Oh no, in order to let latter packets in as soon as possible, we have
> to update qlen immediately.
> 

Absolutely not. You missed something apparently.

You pay the price at each packet enqueue, because you have to compute
the sum of two lengthes, and guess what, if you do this you have a cache
line miss in one of the operand. Your patch as is is suboptimal.

Remember : this batch mode should not change packet queueing at all,
only speed it because of less cache line misses.

--
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