Re: [PATCH net-next V2 3/3] tun: rx batching
On Wed, Jan 04, 2017 at 11:03:32AM +0800, Jason Wang wrote:
> On 2017年01月03日 21:33, Stefan Hajnoczi wrote:
> > On Wed, Dec 28, 2016 at 04:09:31PM +0800, Jason Wang wrote:
> > > +static int tun_rx_batched(struct tun_file *tfile, struct sk_buff *skb,
> > > + int more)
> > > +{
> > > + struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
> > > + struct sk_buff_head process_queue;
> > > + int qlen;
> > > + bool rcv = false;
> > > +
> > > + spin_lock(&queue->lock);
> > Should this be spin_lock_bh()? Below and in tun_get_user() there are
> > explicit local_bh_disable() calls so I guess BHs can interrupt us here
> > and this would deadlock.
>
> sk_write_queue were accessed only in this function which runs under process
> context, so no need for spin_lock_bh() here.
I see, thanks!
Stefan
signature.asc
Description: PGP signature
Re: [PATCH net-next V2 3/3] tun: rx batching
On 2017年01月03日 21:33, Stefan Hajnoczi wrote:
On Wed, Dec 28, 2016 at 04:09:31PM +0800, Jason Wang wrote:
+static int tun_rx_batched(struct tun_file *tfile, struct sk_buff *skb,
+ int more)
+{
+ struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
+ struct sk_buff_head process_queue;
+ int qlen;
+ bool rcv = false;
+
+ spin_lock(&queue->lock);
Should this be spin_lock_bh()? Below and in tun_get_user() there are
explicit local_bh_disable() calls so I guess BHs can interrupt us here
and this would deadlock.
sk_write_queue were accessed only in this function which runs under
process context, so no need for spin_lock_bh() here.
Re: [PATCH net-next V2 3/3] tun: rx batching
On Wed, Dec 28, 2016 at 04:09:31PM +0800, Jason Wang wrote:
> +static int tun_rx_batched(struct tun_file *tfile, struct sk_buff *skb,
> + int more)
> +{
> + struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
> + struct sk_buff_head process_queue;
> + int qlen;
> + bool rcv = false;
> +
> + spin_lock(&queue->lock);
Should this be spin_lock_bh()? Below and in tun_get_user() there are
explicit local_bh_disable() calls so I guess BHs can interrupt us here
and this would deadlock.
signature.asc
Description: PGP signature
Re: [PATCH net-next V2 3/3] tun: rx batching
On 2016年12月30日 00:35, David Miller wrote:
From: Jason Wang
Date: Wed, 28 Dec 2016 16:09:31 +0800
+ spin_lock(&queue->lock);
+ qlen = skb_queue_len(queue);
+ if (qlen > rx_batched)
+ goto drop;
+ __skb_queue_tail(queue, skb);
+ if (!more || qlen + 1 > rx_batched) {
+ __skb_queue_head_init(&process_queue);
+ skb_queue_splice_tail_init(queue, &process_queue);
+ rcv = true;
+ }
+ spin_unlock(&queue->lock);
Since you always clear the 'queue' when you insert the skb that hits
the limit, I don't see how the "goto drop" path can be possibly taken.
True, will fix this.
Thanks
Re: [PATCH net-next V2 3/3] tun: rx batching
From: Jason Wang
Date: Wed, 28 Dec 2016 16:09:31 +0800
> + spin_lock(&queue->lock);
> + qlen = skb_queue_len(queue);
> + if (qlen > rx_batched)
> + goto drop;
> + __skb_queue_tail(queue, skb);
> + if (!more || qlen + 1 > rx_batched) {
> + __skb_queue_head_init(&process_queue);
> + skb_queue_splice_tail_init(queue, &process_queue);
> + rcv = true;
> + }
> + spin_unlock(&queue->lock);
Since you always clear the 'queue' when you insert the skb that hits
the limit, I don't see how the "goto drop" path can be possibly taken.
