[net-next,06/11] tuntap: split out XDP logic
diff mbox series

Message ID 20180906040526.22518-7-jasowang@redhat.com
State New
Headers show
Series
  • Vhost_net TX batching
Related show

Commit Message

Jason Wang Sept. 6, 2018, 4:05 a.m. UTC
This patch split out XDP logic into a single function. This make it to
be reused by XDP batching path in the following patch.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/tun.c | 84 ++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 33 deletions(-)

Comments

Michael S. Tsirkin Sept. 6, 2018, 5:21 p.m. UTC | #1
On Thu, Sep 06, 2018 at 12:05:21PM +0800, Jason Wang wrote:
> This patch split out XDP logic into a single function. This make it to
> be reused by XDP batching path in the following patch.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/tun.c | 84 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 389aa0727cc6..21b125020b3b 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1635,6 +1635,44 @@ static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
>  	return true;
>  }
>  
> +static u32 tun_do_xdp(struct tun_struct *tun,
> +		      struct tun_file *tfile,
> +		      struct bpf_prog *xdp_prog,
> +		      struct xdp_buff *xdp,
> +		      int *err)
> +{
> +	u32 act = bpf_prog_run_xdp(xdp_prog, xdp);
> +
> +	switch (act) {
> +	case XDP_REDIRECT:
> +		*err = xdp_do_redirect(tun->dev, xdp, xdp_prog);
> +		xdp_do_flush_map();
> +		if (*err)
> +			break;
> +		goto out;
> +	case XDP_TX:
> +		*err = tun_xdp_tx(tun->dev, xdp);
> +		if (*err < 0)
> +			break;
> +		*err = 0;
> +		goto out;
> +	case XDP_PASS:
> +		goto out;

Do we need goto? why not just return?

> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +		/* fall through */
> +	case XDP_ABORTED:
> +		trace_xdp_exception(tun->dev, xdp_prog, act);
> +		/* fall through */
> +	case XDP_DROP:
> +		break;
> +	}
> +
> +	put_page(virt_to_head_page(xdp->data_hard_start));

put here because caller does get_page :( Not pretty.
I'd move this out to the caller.

> +out:
> +	return act;

How about combining err and act? err is < 0 XDP_PASS is > 0.
No need for pointers then.

> +}
> +
>  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  				     struct tun_file *tfile,
>  				     struct iov_iter *from,
> @@ -1645,10 +1683,10 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	struct sk_buff *skb = NULL;
>  	struct bpf_prog *xdp_prog;
>  	int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> -	unsigned int delta = 0;
>  	char *buf;
>  	size_t copied;
> -	int err, pad = TUN_RX_PAD;
> +	int pad = TUN_RX_PAD;
> +	int err = 0;
>  
>  	rcu_read_lock();
>  	xdp_prog = rcu_dereference(tun->xdp_prog);
> @@ -1685,9 +1723,8 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  	local_bh_disable();
>  	rcu_read_lock();
>  	xdp_prog = rcu_dereference(tun->xdp_prog);
> -	if (xdp_prog && !*skb_xdp) {
> +	if (xdp_prog) {
>  		struct xdp_buff xdp;
> -		void *orig_data;
>  		u32 act;
>  
>  		xdp.data_hard_start = buf;
> @@ -1695,33 +1732,14 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  		xdp_set_data_meta_invalid(&xdp);
>  		xdp.data_end = xdp.data + len;
>  		xdp.rxq = &tfile->xdp_rxq;
> -		orig_data = xdp.data;
> -		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> -
> -		switch (act) {
> -		case XDP_REDIRECT:
> -			err = xdp_do_redirect(tun->dev, &xdp, xdp_prog);
> -			xdp_do_flush_map();
> -			if (err)
> -				goto err_xdp;
> -			goto out;
> -		case XDP_TX:
> -			if (tun_xdp_tx(tun->dev, &xdp) < 0)
> -				goto err_xdp;
> -			goto out;
> -		case XDP_PASS:
> -			delta = orig_data - xdp.data;
> -			len = xdp.data_end - xdp.data;
> -			break;
> -		default:
> -			bpf_warn_invalid_xdp_action(act);
> -			/* fall through */
> -		case XDP_ABORTED:
> -			trace_xdp_exception(tun->dev, xdp_prog, act);
> -			/* fall through */
> -		case XDP_DROP:
> +		act = tun_do_xdp(tun, tfile, xdp_prog, &xdp, &err);
> +		if (err)
>  			goto err_xdp;
> -		}
> +		if (act != XDP_PASS)
> +			goto out;

likely?

> +
> +		pad = xdp.data - xdp.data_hard_start;
> +		len = xdp.data_end - xdp.data;
>  	}
>  	rcu_read_unlock();
>  	local_bh_enable();
> @@ -1729,18 +1747,18 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  build:
>  	skb = build_skb(buf, buflen);
>  	if (!skb) {
> +		put_page(alloc_frag->page);
>  		skb = ERR_PTR(-ENOMEM);
>  		goto out;
>  	}
>  
> -	skb_reserve(skb, pad - delta);
> +	skb_reserve(skb, pad);
>  	skb_put(skb, len);
>  
>  	return skb;
>  
>  err_xdp:
> -	alloc_frag->offset -= buflen;
> -	put_page(alloc_frag->page);
> +	this_cpu_inc(tun->pcpu_stats->rx_dropped);


This fixes bug in previous patch which dropped it. OK :)

>  out:
>  	rcu_read_unlock();
>  	local_bh_enable();
> -- 
> 2.17.1
Jason Wang Sept. 7, 2018, 3:29 a.m. UTC | #2
On 2018年09月07日 01:21, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2018 at 12:05:21PM +0800, Jason Wang wrote:
>> This patch split out XDP logic into a single function. This make it to
>> be reused by XDP batching path in the following patch.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/net/tun.c | 84 ++++++++++++++++++++++++++++-------------------
>>   1 file changed, 51 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 389aa0727cc6..21b125020b3b 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1635,6 +1635,44 @@ static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
>>   	return true;
>>   }
>>   
>> +static u32 tun_do_xdp(struct tun_struct *tun,
>> +		      struct tun_file *tfile,
>> +		      struct bpf_prog *xdp_prog,
>> +		      struct xdp_buff *xdp,
>> +		      int *err)
>> +{
>> +	u32 act = bpf_prog_run_xdp(xdp_prog, xdp);
>> +
>> +	switch (act) {
>> +	case XDP_REDIRECT:
>> +		*err = xdp_do_redirect(tun->dev, xdp, xdp_prog);
>> +		xdp_do_flush_map();
>> +		if (*err)
>> +			break;
>> +		goto out;
>> +	case XDP_TX:
>> +		*err = tun_xdp_tx(tun->dev, xdp);
>> +		if (*err < 0)
>> +			break;
>> +		*err = 0;
>> +		goto out;
>> +	case XDP_PASS:
>> +		goto out;
> Do we need goto? why not just return?

I don't see any difference.

>
>> +	default:
>> +		bpf_warn_invalid_xdp_action(act);
>> +		/* fall through */
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(tun->dev, xdp_prog, act);
>> +		/* fall through */
>> +	case XDP_DROP:
>> +		break;
>> +	}
>> +
>> +	put_page(virt_to_head_page(xdp->data_hard_start));
> put here because caller does get_page :( Not pretty.
> I'd move this out to the caller.

Then we need a switch in the caller, not sure it's better.

>
>> +out:
>> +	return act;
> How about combining err and act? err is < 0 XDP_PASS is > 0.
> No need for pointers then.

Ok.

>
>> +}
>> +
>>   static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>>   				     struct tun_file *tfile,
>>   				     struct iov_iter *from,
>> @@ -1645,10 +1683,10 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>>   	struct sk_buff *skb = NULL;
>>   	struct bpf_prog *xdp_prog;
>>   	int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> -	unsigned int delta = 0;
>>   	char *buf;
>>   	size_t copied;
>> -	int err, pad = TUN_RX_PAD;
>> +	int pad = TUN_RX_PAD;
>> +	int err = 0;
>>   
>>   	rcu_read_lock();
>>   	xdp_prog = rcu_dereference(tun->xdp_prog);
>> @@ -1685,9 +1723,8 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>>   	local_bh_disable();
>>   	rcu_read_lock();
>>   	xdp_prog = rcu_dereference(tun->xdp_prog);
>> -	if (xdp_prog && !*skb_xdp) {
>> +	if (xdp_prog) {
>>   		struct xdp_buff xdp;
>> -		void *orig_data;
>>   		u32 act;
>>   
>>   		xdp.data_hard_start = buf;
>> @@ -1695,33 +1732,14 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>>   		xdp_set_data_meta_invalid(&xdp);
>>   		xdp.data_end = xdp.data + len;
>>   		xdp.rxq = &tfile->xdp_rxq;
>> -		orig_data = xdp.data;
>> -		act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> -
>> -		switch (act) {
>> -		case XDP_REDIRECT:
>> -			err = xdp_do_redirect(tun->dev, &xdp, xdp_prog);
>> -			xdp_do_flush_map();
>> -			if (err)
>> -				goto err_xdp;
>> -			goto out;
>> -		case XDP_TX:
>> -			if (tun_xdp_tx(tun->dev, &xdp) < 0)
>> -				goto err_xdp;
>> -			goto out;
>> -		case XDP_PASS:
>> -			delta = orig_data - xdp.data;
>> -			len = xdp.data_end - xdp.data;
>> -			break;
>> -		default:
>> -			bpf_warn_invalid_xdp_action(act);
>> -			/* fall through */
>> -		case XDP_ABORTED:
>> -			trace_xdp_exception(tun->dev, xdp_prog, act);
>> -			/* fall through */
>> -		case XDP_DROP:
>> +		act = tun_do_xdp(tun, tfile, xdp_prog, &xdp, &err);
>> +		if (err)
>>   			goto err_xdp;
>> -		}
>> +		if (act != XDP_PASS)
>> +			goto out;
> likely?

It depends on the XDP program, so I tend not to use it.

>
>> +
>> +		pad = xdp.data - xdp.data_hard_start;
>> +		len = xdp.data_end - xdp.data;
>>   	}
>>   	rcu_read_unlock();
>>   	local_bh_enable();
>> @@ -1729,18 +1747,18 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>>   build:
>>   	skb = build_skb(buf, buflen);
>>   	if (!skb) {
>> +		put_page(alloc_frag->page);
>>   		skb = ERR_PTR(-ENOMEM);
>>   		goto out;
>>   	}
>>   
>> -	skb_reserve(skb, pad - delta);
>> +	skb_reserve(skb, pad);
>>   	skb_put(skb, len);
>>   
>>   	return skb;
>>   
>>   err_xdp:
>> -	alloc_frag->offset -= buflen;
>> -	put_page(alloc_frag->page);
>> +	this_cpu_inc(tun->pcpu_stats->rx_dropped);
>
> This fixes bug in previous patch which dropped it. OK :)

Yes, but let me move this to the buggy patch.

Thanks

>>   out:
>>   	rcu_read_unlock();
>>   	local_bh_enable();
>> -- 
>> 2.17.1
Michael S. Tsirkin Sept. 7, 2018, 2:16 p.m. UTC | #3
On Fri, Sep 07, 2018 at 11:29:34AM +0800, Jason Wang wrote:
> > > +		if (act != XDP_PASS)
> > > +			goto out;
> > likely?
> 
> It depends on the XDP program, so I tend not to use it.

Point is XDP_PASS is already slow.
Jason Wang Sept. 10, 2018, 3:43 a.m. UTC | #4
On 2018年09月07日 22:16, Michael S. Tsirkin wrote:
> On Fri, Sep 07, 2018 at 11:29:34AM +0800, Jason Wang wrote:
>>>> +		if (act != XDP_PASS)
>>>> +			goto out;
>>> likely?
>> It depends on the XDP program, so I tend not to use it.
> Point is XDP_PASS is already slow.
>

Ok.

Thanks

Patch
diff mbox series

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 389aa0727cc6..21b125020b3b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1635,6 +1635,44 @@  static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
 	return true;
 }
 
+static u32 tun_do_xdp(struct tun_struct *tun,
+		      struct tun_file *tfile,
+		      struct bpf_prog *xdp_prog,
+		      struct xdp_buff *xdp,
+		      int *err)
+{
+	u32 act = bpf_prog_run_xdp(xdp_prog, xdp);
+
+	switch (act) {
+	case XDP_REDIRECT:
+		*err = xdp_do_redirect(tun->dev, xdp, xdp_prog);
+		xdp_do_flush_map();
+		if (*err)
+			break;
+		goto out;
+	case XDP_TX:
+		*err = tun_xdp_tx(tun->dev, xdp);
+		if (*err < 0)
+			break;
+		*err = 0;
+		goto out;
+	case XDP_PASS:
+		goto out;
+	default:
+		bpf_warn_invalid_xdp_action(act);
+		/* fall through */
+	case XDP_ABORTED:
+		trace_xdp_exception(tun->dev, xdp_prog, act);
+		/* fall through */
+	case XDP_DROP:
+		break;
+	}
+
+	put_page(virt_to_head_page(xdp->data_hard_start));
+out:
+	return act;
+}
+
 static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 				     struct tun_file *tfile,
 				     struct iov_iter *from,
@@ -1645,10 +1683,10 @@  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 	struct sk_buff *skb = NULL;
 	struct bpf_prog *xdp_prog;
 	int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	unsigned int delta = 0;
 	char *buf;
 	size_t copied;
-	int err, pad = TUN_RX_PAD;
+	int pad = TUN_RX_PAD;
+	int err = 0;
 
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(tun->xdp_prog);
@@ -1685,9 +1723,8 @@  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 	local_bh_disable();
 	rcu_read_lock();
 	xdp_prog = rcu_dereference(tun->xdp_prog);
-	if (xdp_prog && !*skb_xdp) {
+	if (xdp_prog) {
 		struct xdp_buff xdp;
-		void *orig_data;
 		u32 act;
 
 		xdp.data_hard_start = buf;
@@ -1695,33 +1732,14 @@  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 		xdp_set_data_meta_invalid(&xdp);
 		xdp.data_end = xdp.data + len;
 		xdp.rxq = &tfile->xdp_rxq;
-		orig_data = xdp.data;
-		act = bpf_prog_run_xdp(xdp_prog, &xdp);
-
-		switch (act) {
-		case XDP_REDIRECT:
-			err = xdp_do_redirect(tun->dev, &xdp, xdp_prog);
-			xdp_do_flush_map();
-			if (err)
-				goto err_xdp;
-			goto out;
-		case XDP_TX:
-			if (tun_xdp_tx(tun->dev, &xdp) < 0)
-				goto err_xdp;
-			goto out;
-		case XDP_PASS:
-			delta = orig_data - xdp.data;
-			len = xdp.data_end - xdp.data;
-			break;
-		default:
-			bpf_warn_invalid_xdp_action(act);
-			/* fall through */
-		case XDP_ABORTED:
-			trace_xdp_exception(tun->dev, xdp_prog, act);
-			/* fall through */
-		case XDP_DROP:
+		act = tun_do_xdp(tun, tfile, xdp_prog, &xdp, &err);
+		if (err)
 			goto err_xdp;
-		}
+		if (act != XDP_PASS)
+			goto out;
+
+		pad = xdp.data - xdp.data_hard_start;
+		len = xdp.data_end - xdp.data;
 	}
 	rcu_read_unlock();
 	local_bh_enable();
@@ -1729,18 +1747,18 @@  static struct sk_buff *tun_build_skb(struct tun_struct *tun,
 build:
 	skb = build_skb(buf, buflen);
 	if (!skb) {
+		put_page(alloc_frag->page);
 		skb = ERR_PTR(-ENOMEM);
 		goto out;
 	}
 
-	skb_reserve(skb, pad - delta);
+	skb_reserve(skb, pad);
 	skb_put(skb, len);
 
 	return skb;
 
 err_xdp:
-	alloc_frag->offset -= buflen;
-	put_page(alloc_frag->page);
+	this_cpu_inc(tun->pcpu_stats->rx_dropped);
 out:
 	rcu_read_unlock();
 	local_bh_enable();