* [PATCH bpf-next] virtio_net: add XDP meta data support
@ 2019-06-27 8:06 Yuya Kusakabe
2019-07-01 9:30 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-06-27 8:06 UTC (permalink / raw)
To: davem
Cc: Michael S. Tsirkin, Jason Wang, Alexei Starovoitov,
Daniel Borkmann, Jakub Kicinski, Jesper Dangaard Brouer,
John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song,
netdev, Yuya Kusakabe
This adds XDP meta data support to both receive_small() and
receive_mergeable().
Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
---
drivers/net/virtio_net.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f3de0ac8b0b..e787657fc568 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize,
- bool hdr_valid)
+ bool hdr_valid, unsigned int metasize)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -393,17 +393,25 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- if (hdr_valid)
+ if (hdr_valid && !metasize)
memcpy(hdr, p, hdr_len);
len -= hdr_len;
offset += hdr_padded_len;
p += hdr_padded_len;
- copy = len;
+ copy = len + metasize;
if (copy > skb_tailroom(skb))
copy = skb_tailroom(skb);
- skb_put_data(skb, p, copy);
+
+ if (metasize) {
+ skb_put_data(skb, p - metasize, copy);
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ copy -= metasize;
+ } else {
+ skb_put_data(skb, p, copy);
+ }
len -= copy;
offset += copy;
@@ -644,6 +652,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int delta = 0;
struct page *xdp_page;
int err;
+ unsigned int metasize = 0;
len -= vi->hdr_len;
stats->bytes += len;
@@ -683,8 +692,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
/* Recalculate length in case bpf program changed it */
delta = orig_data - xdp.data;
len = xdp.data_end - xdp.data;
+ metasize = xdp.data - xdp.data_meta;
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -735,11 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
}
skb_reserve(skb, headroom - delta);
skb_put(skb, len);
- if (!delta) {
+ if (!delta && !metasize) {
buf += header_offset;
memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
+ if (metasize)
+ skb_metadata_set(skb, metasize);
+
err:
return skb;
@@ -761,7 +775,7 @@ static struct sk_buff *receive_big(struct net_device *dev,
{
struct page *page = buf;
struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
- PAGE_SIZE, true);
+ PAGE_SIZE, true, 0);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -793,6 +807,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
int err;
+ unsigned int metasize = 0;
head_skb = NULL;
stats->bytes += len - vi->hdr_len;
@@ -839,8 +854,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
data = page_address(xdp_page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + (len - vi->hdr_len);
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -859,18 +874,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
* adjusted
*/
len = xdp.data_end - xdp.data + vi->hdr_len;
+ metasize = xdp.data - xdp.data_meta;
/* We can only create skb based on xdp_page. */
if (unlikely(xdp_page != page)) {
rcu_read_unlock();
put_page(page);
head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len,
- PAGE_SIZE, false);
+ offset, len,
+ PAGE_SIZE, false, metasize);
return head_skb;
}
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -921,7 +938,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
+ metasize);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.20.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] virtio_net: add XDP meta data support
2019-06-27 8:06 [PATCH bpf-next] virtio_net: add XDP meta data support Yuya Kusakabe
@ 2019-07-01 9:30 ` Jason Wang
2019-07-02 1:00 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2019-07-01 9:30 UTC (permalink / raw)
To: Yuya Kusakabe, davem
Cc: Michael S. Tsirkin, Alexei Starovoitov, Daniel Borkmann,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
Martin KaFai Lau, Song Liu, Yonghong Song, netdev
On 2019/6/27 下午4:06, Yuya Kusakabe wrote:
> This adds XDP meta data support to both receive_small() and
> receive_mergeable().
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> ---
> drivers/net/virtio_net.c | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f3de0ac8b0b..e787657fc568 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> struct page *page, unsigned int offset,
> unsigned int len, unsigned int truesize,
> - bool hdr_valid)
> + bool hdr_valid, unsigned int metasize)
> {
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> @@ -393,17 +393,25 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> - if (hdr_valid)
> + if (hdr_valid && !metasize)
> memcpy(hdr, p, hdr_len);
>
> len -= hdr_len;
> offset += hdr_padded_len;
> p += hdr_padded_len;
>
> - copy = len;
> + copy = len + metasize;
> if (copy > skb_tailroom(skb))
> copy = skb_tailroom(skb);
> - skb_put_data(skb, p, copy);
> +
> + if (metasize) {
> + skb_put_data(skb, p - metasize, copy);
I would rather keep copy untouched above, and use copy + metasize here,
then you can save the following decrement as well. Or tweak the caller
the count the meta in to offset, then we need only deal with skb_pull()
and skb_metadata_set() here.
> + __skb_pull(skb, metasize);
> + skb_metadata_set(skb, metasize);
> + copy -= metasize;
> + } else {
> + skb_put_data(skb, p, copy);
> + }
>
> len -= copy;
> offset += copy;
> @@ -644,6 +652,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> unsigned int delta = 0;
> struct page *xdp_page;
> int err;
> + unsigned int metasize = 0;
>
> len -= vi->hdr_len;
> stats->bytes += len;
> @@ -683,8 +692,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + len;
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
> /* Recalculate length in case bpf program changed it */
> delta = orig_data - xdp.data;
> len = xdp.data_end - xdp.data;
> + metasize = xdp.data - xdp.data_meta;
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
Why need this?
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -735,11 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
> }
> skb_reserve(skb, headroom - delta);
> skb_put(skb, len);
> - if (!delta) {
> + if (!delta && !metasize) {
> buf += header_offset;
> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
Is there any method to preserve the vnet header here? We probably don't
want to lose it for XDP_PASS when packet is not modified.
>
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> err:
> return skb;
>
> @@ -761,7 +775,7 @@ static struct sk_buff *receive_big(struct net_device *dev,
> {
> struct page *page = buf;
> struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> - PAGE_SIZE, true);
> + PAGE_SIZE, true, 0);
>
> stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> @@ -793,6 +807,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> unsigned int truesize;
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> int err;
> + unsigned int metasize = 0;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> @@ -839,8 +854,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> data = page_address(xdp_page) + offset;
> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> xdp.data = data + vi->hdr_len;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + (len - vi->hdr_len);
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -859,18 +874,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> * adjusted
> */
> len = xdp.data_end - xdp.data + vi->hdr_len;
> + metasize = xdp.data - xdp.data_meta;
> /* We can only create skb based on xdp_page. */
> if (unlikely(xdp_page != page)) {
> rcu_read_unlock();
> put_page(page);
> head_skb = page_to_skb(vi, rq, xdp_page,
> - offset, len,
> - PAGE_SIZE, false);
> + offset, len,
> + PAGE_SIZE, false, metasize);
Indentation is wired.
Thanks
> return head_skb;
> }
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -921,7 +938,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_skb;
> }
>
> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> + metasize);
> curr_skb = head_skb;
>
> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next] virtio_net: add XDP meta data support
2019-07-01 9:30 ` Jason Wang
@ 2019-07-02 1:00 ` Yuya Kusakabe
2019-07-02 3:15 ` [PATCH bpf-next v2] " Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-07-02 1:00 UTC (permalink / raw)
To: Jason Wang, davem
Cc: Michael S. Tsirkin, Alexei Starovoitov, Daniel Borkmann,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
Martin KaFai Lau, Song Liu, Yonghong Song, netdev
On 7/1/19 6:30 PM, Jason Wang wrote:
>
> On 2019/6/27 下午4:06, Yuya Kusakabe wrote:
>> This adds XDP meta data support to both receive_small() and
>> receive_mergeable().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> drivers/net/virtio_net.c | 40 +++++++++++++++++++++++++++++-----------
>> 1 file changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4f3de0ac8b0b..e787657fc568 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,17 +393,25 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> offset += hdr_padded_len;
>> p += hdr_padded_len;
>> - copy = len;
>> + copy = len + metasize;
>> if (copy > skb_tailroom(skb))
>> copy = skb_tailroom(skb);
>> - skb_put_data(skb, p, copy);
>> +
>> + if (metasize) {
>> + skb_put_data(skb, p - metasize, copy);
>
>
> I would rather keep copy untouched above, and use copy + metasize here, then you can save the following decrement as well. Or tweak the caller the count the meta in to offset, then we need only deal with skb_pull() and skb_metadata_set() here.
I think the latter is better, because copy + metasize must be smaller than skb tailroom size.
>
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + copy -= metasize;
>> + } else {
>> + skb_put_data(skb, p, copy);
>> + }
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +652,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,8 +692,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>
>
> Why need this?
Because virtnet_xdp_xmit() doesn't support XDP meta data as below. And I suppose that we don't need to support XDP metadata for XDP_TX.
static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
struct send_queue *sq,
struct xdp_frame *xdpf)
{
struct virtio_net_hdr_mrg_rxbuf *hdr;
int err;
/* virtqueue want to use data area in-front of packet */
if (unlikely(xdpf->metasize > 0))
return -EOPNOTSUPP;
>
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -735,11 +746,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> }
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> - if (!delta) {
>> + if (!delta && !metasize) {
>> buf += header_offset;
>> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
>
> Is there any method to preserve the vnet header here? We probably don't want to lose it for XDP_PASS when packet is not modified.
I'll try to keep the vnet header with moving the meta data to the front of the vnet header.
>
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -761,7 +775,7 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> {
>> struct page *page = buf;
>> struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +807,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +854,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -859,18 +874,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjusted
>> */
>> len = xdp.data_end - xdp.data + vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> /* We can only create skb based on xdp_page. */
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + offset, len,
>> + PAGE_SIZE, false, metasize);
>
>
> Indentation is wired.
Sorry. I'll fix it.
>
> Thanks
>
>
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +938,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next v2] virtio_net: add XDP meta data support
2019-07-02 1:00 ` Yuya Kusakabe
@ 2019-07-02 3:15 ` Yuya Kusakabe
2019-07-02 3:59 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-07-02 3:15 UTC (permalink / raw)
To: yuya.kusakabe
Cc: ast, daniel, davem, hawk, jakub.kicinski, jasowang,
john.fastabend, kafai, mst, netdev, songliubraving, yhs
This adds XDP meta data support to both receive_small() and
receive_mergeable().
Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
---
v2:
- keep copy untouched in page_to_skb().
- preserve the vnet header in receive_small().
- fix indentation.
---
drivers/net/virtio_net.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f3de0ac8b0b..2ebabb08b824 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize,
- bool hdr_valid)
+ bool hdr_valid, unsigned int metasize)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- if (hdr_valid)
+ if (hdr_valid && !metasize)
memcpy(hdr, p, hdr_len);
len -= hdr_len;
@@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
copy = skb_tailroom(skb);
skb_put_data(skb, p, copy);
+ if (metasize) {
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ }
+
len -= copy;
offset += copy;
@@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int delta = 0;
struct page *xdp_page;
int err;
+ unsigned int metasize = 0;
len -= vi->hdr_len;
stats->bytes += len;
@@ -683,8 +689,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -695,9 +701,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
/* Recalculate length in case bpf program changed it */
delta = orig_data - xdp.data;
len = xdp.data_end - xdp.data;
+ metasize = xdp.data - xdp.data_meta;
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -740,6 +748,9 @@ static struct sk_buff *receive_small(struct net_device *dev,
memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
+ if (metasize)
+ skb_metadata_set(skb, metasize);
+
err:
return skb;
@@ -760,8 +771,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
- PAGE_SIZE, true);
+ struct sk_buff *skb =
+ page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -793,6 +804,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
int err;
+ unsigned int metasize = 0;
head_skb = NULL;
stats->bytes += len - vi->hdr_len;
@@ -839,8 +851,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
data = page_address(xdp_page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + (len - vi->hdr_len);
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -852,8 +864,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
* adjustments. Note other cases do not build an
* skb and avoid using offset
*/
- offset = xdp.data -
- page_address(xdp_page) - vi->hdr_len;
+ metasize = xdp.data - xdp.data_meta;
+ offset = xdp.data - page_address(xdp_page) -
+ vi->hdr_len - metasize;
/* recalculate len if xdp.data or xdp.data_end were
* adjusted
@@ -863,14 +876,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
if (unlikely(xdp_page != page)) {
rcu_read_unlock();
put_page(page);
- head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len,
- PAGE_SIZE, false);
+ head_skb = page_to_skb(vi, rq, xdp_page, offset,
+ len, PAGE_SIZE, false,
+ metasize);
return head_skb;
}
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -921,7 +935,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
+ metasize);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.20.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v2] virtio_net: add XDP meta data support
2019-07-02 3:15 ` [PATCH bpf-next v2] " Yuya Kusakabe
@ 2019-07-02 3:59 ` Jason Wang
2019-07-02 5:15 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2019-07-02 3:59 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
On 2019/7/2 上午11:15, Yuya Kusakabe wrote:
> This adds XDP meta data support to both receive_small() and
> receive_mergeable().
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> ---
> v2:
> - keep copy untouched in page_to_skb().
> - preserve the vnet header in receive_small().
> - fix indentation.
> ---
> drivers/net/virtio_net.c | 39 +++++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f3de0ac8b0b..2ebabb08b824 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> struct page *page, unsigned int offset,
> unsigned int len, unsigned int truesize,
> - bool hdr_valid)
> + bool hdr_valid, unsigned int metasize)
> {
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> - if (hdr_valid)
> + if (hdr_valid && !metasize)
> memcpy(hdr, p, hdr_len);
>
> len -= hdr_len;
> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> copy = skb_tailroom(skb);
> skb_put_data(skb, p, copy);
>
> + if (metasize) {
> + __skb_pull(skb, metasize);
> + skb_metadata_set(skb, metasize);
> + }
> +
> len -= copy;
> offset += copy;
>
> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> unsigned int delta = 0;
> struct page *xdp_page;
> int err;
> + unsigned int metasize = 0;
>
> len -= vi->hdr_len;
> stats->bytes += len;
> @@ -683,8 +689,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + len;
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -695,9 +701,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
> /* Recalculate length in case bpf program changed it */
> delta = orig_data - xdp.data;
> len = xdp.data_end - xdp.data;
> + metasize = xdp.data - xdp.data_meta;
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -740,6 +748,9 @@ static struct sk_buff *receive_small(struct net_device *dev,
> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
I wonder whether or not it's as simple as this. Consider bpf may adjust
meta, it looks to me that the vnet header will be overwrite here? If
yes, we probably need to have a device specific value then bpf can move
the device metadata like vnet header for us?
Thanks
>
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> err:
> return skb;
>
> @@ -760,8 +771,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> - PAGE_SIZE, true);
> + struct sk_buff *skb =
> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>
> stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> @@ -793,6 +804,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> unsigned int truesize;
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> int err;
> + unsigned int metasize = 0;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> @@ -839,8 +851,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> data = page_address(xdp_page) + offset;
> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> xdp.data = data + vi->hdr_len;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + (len - vi->hdr_len);
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -852,8 +864,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> * adjustments. Note other cases do not build an
> * skb and avoid using offset
> */
> - offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> + metasize = xdp.data - xdp.data_meta;
> + offset = xdp.data - page_address(xdp_page) -
> + vi->hdr_len - metasize;
>
> /* recalculate len if xdp.data or xdp.data_end were
> * adjusted
> @@ -863,14 +876,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> if (unlikely(xdp_page != page)) {
> rcu_read_unlock();
> put_page(page);
> - head_skb = page_to_skb(vi, rq, xdp_page,
> - offset, len,
> - PAGE_SIZE, false);
> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> + len, PAGE_SIZE, false,
> + metasize);
> return head_skb;
> }
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -921,7 +935,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_skb;
> }
>
> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> + metasize);
> curr_skb = head_skb;
>
> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v2] virtio_net: add XDP meta data support
2019-07-02 3:59 ` Jason Wang
@ 2019-07-02 5:15 ` Yuya Kusakabe
2019-07-02 8:16 ` [PATCH bpf-next v3] " Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-07-02 5:15 UTC (permalink / raw)
To: Jason Wang
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
On 7/2/19 12:59 PM, Jason Wang wrote:
>
> On 2019/7/2 上午11:15, Yuya Kusakabe wrote:
>> This adds XDP meta data support to both receive_small() and
>> receive_mergeable().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> v2:
>> - keep copy untouched in page_to_skb().
>> - preserve the vnet header in receive_small().
>> - fix indentation.
>> ---
>> drivers/net/virtio_net.c | 39 +++++++++++++++++++++++++++------------
>> 1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4f3de0ac8b0b..2ebabb08b824 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,8 +689,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -695,9 +701,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -740,6 +748,9 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
>
> I wonder whether or not it's as simple as this. Consider bpf may adjust meta, it looks to me that the vnet header will be overwrite here? If yes, we probably need to have a device specific value then bpf can move the device metadata like vnet header for us?>
> Thanks
Yes, the vnet header is overwrite by xdp metadata. I'll fix it.
Thanks.
>
>
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +771,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +804,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +851,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -852,8 +864,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>> @@ -863,14 +876,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +935,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-02 5:15 ` Yuya Kusakabe
@ 2019-07-02 8:16 ` Yuya Kusakabe
2019-07-02 8:33 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-07-02 8:16 UTC (permalink / raw)
To: jasowang
Cc: yuya.kusakabe, ast, daniel, davem, hawk, jakub.kicinski,
john.fastabend, kafai, mst, netdev, songliubraving, yhs
This adds XDP meta data support to both receive_small() and
receive_mergeable().
Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
---
v3:
- fix preserve the vnet header in receive_small().
v2:
- keep copy untouched in page_to_skb().
- preserve the vnet header in receive_small().
- fix indentation.
---
drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
1 file changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f3de0ac8b0b..03a1ae6fe267 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize,
- bool hdr_valid)
+ bool hdr_valid, unsigned int metasize)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- if (hdr_valid)
+ if (hdr_valid && !metasize)
memcpy(hdr, p, hdr_len);
len -= hdr_len;
@@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
copy = skb_tailroom(skb);
skb_put_data(skb, p, copy);
+ if (metasize) {
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ }
+
len -= copy;
offset += copy;
@@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int delta = 0;
struct page *xdp_page;
int err;
+ unsigned int metasize = 0;
len -= vi->hdr_len;
stats->bytes += len;
@@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
+ /* Copy the vnet header to the front of data_hard_start to avoid
+ * overwriting by XDP meta data */
+ memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
act = bpf_prog_run_xdp(xdp_prog, &xdp);
stats->xdp_packets++;
@@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
/* Recalculate length in case bpf program changed it */
delta = orig_data - xdp.data;
len = xdp.data_end - xdp.data;
+ metasize = xdp.data - xdp.data_meta;
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
skb_reserve(skb, headroom - delta);
skb_put(skb, len);
if (!delta) {
- buf += header_offset;
- memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
+ memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
+ if (metasize)
+ skb_metadata_set(skb, metasize);
+
err:
return skb;
@@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
- PAGE_SIZE, true);
+ struct sk_buff *skb =
+ page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
int err;
+ unsigned int metasize = 0;
head_skb = NULL;
stats->bytes += len - vi->hdr_len;
@@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
data = page_address(xdp_page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + (len - vi->hdr_len);
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
* adjustments. Note other cases do not build an
* skb and avoid using offset
*/
- offset = xdp.data -
- page_address(xdp_page) - vi->hdr_len;
+ metasize = xdp.data - xdp.data_meta;
+ offset = xdp.data - page_address(xdp_page) -
+ vi->hdr_len - metasize;
/* recalculate len if xdp.data or xdp.data_end were
* adjusted
@@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
if (unlikely(xdp_page != page)) {
rcu_read_unlock();
put_page(page);
- head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len,
- PAGE_SIZE, false);
+ head_skb = page_to_skb(vi, rq, xdp_page, offset,
+ len, PAGE_SIZE, false,
+ metasize);
return head_skb;
}
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
+ metasize);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.20.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-02 8:16 ` [PATCH bpf-next v3] " Yuya Kusakabe
@ 2019-07-02 8:33 ` Jason Wang
2019-07-02 14:11 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2019-07-02 8:33 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
> This adds XDP meta data support to both receive_small() and
> receive_mergeable().
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> ---
> v3:
> - fix preserve the vnet header in receive_small().
> v2:
> - keep copy untouched in page_to_skb().
> - preserve the vnet header in receive_small().
> - fix indentation.
> ---
> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
> 1 file changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f3de0ac8b0b..03a1ae6fe267 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> struct page *page, unsigned int offset,
> unsigned int len, unsigned int truesize,
> - bool hdr_valid)
> + bool hdr_valid, unsigned int metasize)
> {
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> - if (hdr_valid)
> + if (hdr_valid && !metasize)
> memcpy(hdr, p, hdr_len);
>
> len -= hdr_len;
> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> copy = skb_tailroom(skb);
> skb_put_data(skb, p, copy);
>
> + if (metasize) {
> + __skb_pull(skb, metasize);
> + skb_metadata_set(skb, metasize);
> + }
> +
> len -= copy;
> offset += copy;
>
> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> unsigned int delta = 0;
> struct page *xdp_page;
> int err;
> + unsigned int metasize = 0;
>
> len -= vi->hdr_len;
> stats->bytes += len;
> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + len;
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> + /* Copy the vnet header to the front of data_hard_start to avoid
> + * overwriting by XDP meta data */
> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
What happens if we have a large metadata that occupies all headroom here?
Thanks
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> stats->xdp_packets++;
>
> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
> /* Recalculate length in case bpf program changed it */
> delta = orig_data - xdp.data;
> len = xdp.data_end - xdp.data;
> + metasize = xdp.data - xdp.data_meta;
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
> skb_reserve(skb, headroom - delta);
> skb_put(skb, len);
> if (!delta) {
> - buf += header_offset;
> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> err:
> return skb;
>
> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> - PAGE_SIZE, true);
> + struct sk_buff *skb =
> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>
> stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> unsigned int truesize;
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> int err;
> + unsigned int metasize = 0;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> data = page_address(xdp_page) + offset;
> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> xdp.data = data + vi->hdr_len;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + (len - vi->hdr_len);
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> * adjustments. Note other cases do not build an
> * skb and avoid using offset
> */
> - offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> + metasize = xdp.data - xdp.data_meta;
> + offset = xdp.data - page_address(xdp_page) -
> + vi->hdr_len - metasize;
>
> /* recalculate len if xdp.data or xdp.data_end were
> * adjusted
> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> if (unlikely(xdp_page != page)) {
> rcu_read_unlock();
> put_page(page);
> - head_skb = page_to_skb(vi, rq, xdp_page,
> - offset, len,
> - PAGE_SIZE, false);
> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> + len, PAGE_SIZE, false,
> + metasize);
> return head_skb;
> }
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_skb;
> }
>
> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> + metasize);
> curr_skb = head_skb;
>
> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-02 8:33 ` Jason Wang
@ 2019-07-02 14:11 ` Yuya Kusakabe
2019-07-08 22:38 ` Daniel Borkmann
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2019-07-02 14:11 UTC (permalink / raw)
To: Jason Wang
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
On 7/2/19 5:33 PM, Jason Wang wrote:
>
> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>> This adds XDP meta data support to both receive_small() and
>> receive_mergeable().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> v3:
>> - fix preserve the vnet header in receive_small().
>> v2:
>> - keep copy untouched in page_to_skb().
>> - preserve the vnet header in receive_small().
>> - fix indentation.
>> ---
>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> + /* Copy the vnet header to the front of data_hard_start to avoid
>> + * overwriting by XDP meta data */
>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>
>
> What happens if we have a large metadata that occupies all headroom here?
>
> Thanks
Do you mean a large "XDP" metadata? If a large metadata is a large "XDP" metadata, I think we can not use a metadata that occupies all headroom. The size of metadata limited by bpf_xdp_adjust_meta() as below.
bpf_xdp_adjust_meta() in net/core/filter.c:
if (unlikely((metalen & (sizeof(__u32) - 1)) ||
(metalen > 32)))
return -EACCES;
Thanks.
>
>
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> stats->xdp_packets++;
>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> if (!delta) {
>> - buf += header_offset;
>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-02 14:11 ` Yuya Kusakabe
@ 2019-07-08 22:38 ` Daniel Borkmann
2019-07-09 3:04 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Borkmann @ 2019-07-08 22:38 UTC (permalink / raw)
To: Yuya Kusakabe, Jason Wang
Cc: ast, davem, hawk, jakub.kicinski, john.fastabend, kafai, mst,
netdev, songliubraving, yhs
On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
> On 7/2/19 5:33 PM, Jason Wang wrote:
>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>>> This adds XDP meta data support to both receive_small() and
>>> receive_mergeable().
>>>
>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>>> ---
>>> v3:
>>> - fix preserve the vnet header in receive_small().
>>> v2:
>>> - keep copy untouched in page_to_skb().
>>> - preserve the vnet header in receive_small().
>>> - fix indentation.
>>> ---
>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>> struct receive_queue *rq,
>>> struct page *page, unsigned int offset,
>>> unsigned int len, unsigned int truesize,
>>> - bool hdr_valid)
>>> + bool hdr_valid, unsigned int metasize)
>>> {
>>> struct sk_buff *skb;
>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>> else
>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>> - if (hdr_valid)
>>> + if (hdr_valid && !metasize)
>>> memcpy(hdr, p, hdr_len);
>>> len -= hdr_len;
>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>> copy = skb_tailroom(skb);
>>> skb_put_data(skb, p, copy);
>>> + if (metasize) {
>>> + __skb_pull(skb, metasize);
>>> + skb_metadata_set(skb, metasize);
>>> + }
>>> +
>>> len -= copy;
>>> offset += copy;
>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>> unsigned int delta = 0;
>>> struct page *xdp_page;
>>> int err;
>>> + unsigned int metasize = 0;
>>> len -= vi->hdr_len;
>>> stats->bytes += len;
>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>> - xdp_set_data_meta_invalid(&xdp);
>>> xdp.data_end = xdp.data + len;
>>> + xdp.data_meta = xdp.data;
>>> xdp.rxq = &rq->xdp_rxq;
>>> orig_data = xdp.data;
>>> + /* Copy the vnet header to the front of data_hard_start to avoid
>>> + * overwriting by XDP meta data */
>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
I'm not fully sure if I'm following this one correctly, probably just missing
something. Isn't the vnet header based on how we set up xdp.data_hard_start
earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
vi->hdr_len into the vnet header at that point (given there can be up to 256
bytes of headroom between the two)? If it's relative to xdp.data and headroom
is >0, then BPF prog could otherwise mangle this; something doesn't add up to
me here. Could you clarify? Thx
>> What happens if we have a large metadata that occupies all headroom here?
>>
>> Thanks
>
> Do you mean a large "XDP" metadata? If a large metadata is a large "XDP" metadata, I think we can not use a metadata that occupies all headroom. The size of metadata limited by bpf_xdp_adjust_meta() as below.
> bpf_xdp_adjust_meta() in net/core/filter.c:
> if (unlikely((metalen & (sizeof(__u32) - 1)) ||
> (metalen > 32)))
> return -EACCES;
>
> Thanks.
>
>>
>>
>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>> stats->xdp_packets++;
>>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>> /* Recalculate length in case bpf program changed it */
>>> delta = orig_data - xdp.data;
>>> len = xdp.data_end - xdp.data;
>>> + metasize = xdp.data - xdp.data_meta;
>>> break;
>>> case XDP_TX:
>>> stats->xdp_tx++;
>>> + xdp.data_meta = xdp.data;
>>> xdpf = convert_to_xdp_frame(&xdp);
>>> if (unlikely(!xdpf))
>>> goto err_xdp;
>>> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>> skb_reserve(skb, headroom - delta);
>>> skb_put(skb, len);
>>> if (!delta) {
>>> - buf += header_offset;
>>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>>> + if (metasize)
>>> + skb_metadata_set(skb, metasize);
>>> +
>>> err:
>>> return skb;
>>> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>>> struct virtnet_rq_stats *stats)
>>> {
>>> struct page *page = buf;
>>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>>> - PAGE_SIZE, true);
>>> + struct sk_buff *skb =
>>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>>> stats->bytes += len - vi->hdr_len;
>>> if (unlikely(!skb))
>>> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>> unsigned int truesize;
>>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>>> int err;
>>> + unsigned int metasize = 0;
>>> head_skb = NULL;
>>> stats->bytes += len - vi->hdr_len;
>>> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>> data = page_address(xdp_page) + offset;
>>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>>> xdp.data = data + vi->hdr_len;
>>> - xdp_set_data_meta_invalid(&xdp);
>>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>>> + xdp.data_meta = xdp.data;
>>> xdp.rxq = &rq->xdp_rxq;
>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>> * adjustments. Note other cases do not build an
>>> * skb and avoid using offset
>>> */
>>> - offset = xdp.data -
>>> - page_address(xdp_page) - vi->hdr_len;
>>> + metasize = xdp.data - xdp.data_meta;
>>> + offset = xdp.data - page_address(xdp_page) -
>>> + vi->hdr_len - metasize;
>>> /* recalculate len if xdp.data or xdp.data_end were
>>> * adjusted
>>> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>> if (unlikely(xdp_page != page)) {
>>> rcu_read_unlock();
>>> put_page(page);
>>> - head_skb = page_to_skb(vi, rq, xdp_page,
>>> - offset, len,
>>> - PAGE_SIZE, false);
>>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>>> + len, PAGE_SIZE, false,
>>> + metasize);
>>> return head_skb;
>>> }
>>> break;
>>> case XDP_TX:
>>> stats->xdp_tx++;
>>> + xdp.data_meta = xdp.data;
>>> xdpf = convert_to_xdp_frame(&xdp);
>>> if (unlikely(!xdpf))
>>> goto err_xdp;
>>> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>> goto err_skb;
>>> }
>>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>>> + metasize);
>>> curr_skb = head_skb;
>>> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-08 22:38 ` Daniel Borkmann
@ 2019-07-09 3:04 ` Jason Wang
2019-07-09 20:03 ` Daniel Borkmann
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2019-07-09 3:04 UTC (permalink / raw)
To: Daniel Borkmann, Yuya Kusakabe
Cc: ast, davem, hawk, jakub.kicinski, john.fastabend, kafai, mst,
netdev, songliubraving, yhs
On 2019/7/9 上午6:38, Daniel Borkmann wrote:
> On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
>> On 7/2/19 5:33 PM, Jason Wang wrote:
>>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>>>> This adds XDP meta data support to both receive_small() and
>>>> receive_mergeable().
>>>>
>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>>>> ---
>>>> v3:
>>>> - fix preserve the vnet header in receive_small().
>>>> v2:
>>>> - keep copy untouched in page_to_skb().
>>>> - preserve the vnet header in receive_small().
>>>> - fix indentation.
>>>> ---
>>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>>>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> struct receive_queue *rq,
>>>> struct page *page, unsigned int offset,
>>>> unsigned int len, unsigned int truesize,
>>>> - bool hdr_valid)
>>>> + bool hdr_valid, unsigned int metasize)
>>>> {
>>>> struct sk_buff *skb;
>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> else
>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>> - if (hdr_valid)
>>>> + if (hdr_valid && !metasize)
>>>> memcpy(hdr, p, hdr_len);
>>>> len -= hdr_len;
>>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> copy = skb_tailroom(skb);
>>>> skb_put_data(skb, p, copy);
>>>> + if (metasize) {
>>>> + __skb_pull(skb, metasize);
>>>> + skb_metadata_set(skb, metasize);
>>>> + }
>>>> +
>>>> len -= copy;
>>>> offset += copy;
>>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> unsigned int delta = 0;
>>>> struct page *xdp_page;
>>>> int err;
>>>> + unsigned int metasize = 0;
>>>> len -= vi->hdr_len;
>>>> stats->bytes += len;
>>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>> - xdp_set_data_meta_invalid(&xdp);
>>>> xdp.data_end = xdp.data + len;
>>>> + xdp.data_meta = xdp.data;
>>>> xdp.rxq = &rq->xdp_rxq;
>>>> orig_data = xdp.data;
>>>> + /* Copy the vnet header to the front of data_hard_start to avoid
>>>> + * overwriting by XDP meta data */
>>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
> I'm not fully sure if I'm following this one correctly, probably just missing
> something. Isn't the vnet header based on how we set up xdp.data_hard_start
> earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
> vi->hdr_len into the vnet header at that point (given there can be up to 256
> bytes of headroom between the two)? If it's relative to xdp.data and headroom
> is >0, then BPF prog could otherwise mangle this; something doesn't add up to
> me here. Could you clarify? Thx
Vnet headr sits just in front of xdp.data not xdp.data_hard_start. So it
could be overwrote by metadata, that's why we need a copy here.
Thanks
>
>>> What happens if we have a large metadata that occupies all headroom here?
>>>
>>> Thanks
>> Do you mean a large "XDP" metadata? If a large metadata is a large "XDP" metadata, I think we can not use a metadata that occupies all headroom. The size of metadata limited by bpf_xdp_adjust_meta() as below.
>> bpf_xdp_adjust_meta() in net/core/filter.c:
>> if (unlikely((metalen & (sizeof(__u32) - 1)) ||
>> (metalen > 32)))
>> return -EACCES;
>>
>> Thanks.
>>
>>>
>>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>> stats->xdp_packets++;
>>>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> /* Recalculate length in case bpf program changed it */
>>>> delta = orig_data - xdp.data;
>>>> len = xdp.data_end - xdp.data;
>>>> + metasize = xdp.data - xdp.data_meta;
>>>> break;
>>>> case XDP_TX:
>>>> stats->xdp_tx++;
>>>> + xdp.data_meta = xdp.data;
>>>> xdpf = convert_to_xdp_frame(&xdp);
>>>> if (unlikely(!xdpf))
>>>> goto err_xdp;
>>>> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> skb_reserve(skb, headroom - delta);
>>>> skb_put(skb, len);
>>>> if (!delta) {
>>>> - buf += header_offset;
>>>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>>>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>>>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>>>> + if (metasize)
>>>> + skb_metadata_set(skb, metasize);
>>>> +
>>>> err:
>>>> return skb;
>>>> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>>>> struct virtnet_rq_stats *stats)
>>>> {
>>>> struct page *page = buf;
>>>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>>>> - PAGE_SIZE, true);
>>>> + struct sk_buff *skb =
>>>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>>>> stats->bytes += len - vi->hdr_len;
>>>> if (unlikely(!skb))
>>>> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>> unsigned int truesize;
>>>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>>>> int err;
>>>> + unsigned int metasize = 0;
>>>> head_skb = NULL;
>>>> stats->bytes += len - vi->hdr_len;
>>>> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>> data = page_address(xdp_page) + offset;
>>>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>>>> xdp.data = data + vi->hdr_len;
>>>> - xdp_set_data_meta_invalid(&xdp);
>>>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>>>> + xdp.data_meta = xdp.data;
>>>> xdp.rxq = &rq->xdp_rxq;
>>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>> * adjustments. Note other cases do not build an
>>>> * skb and avoid using offset
>>>> */
>>>> - offset = xdp.data -
>>>> - page_address(xdp_page) - vi->hdr_len;
>>>> + metasize = xdp.data - xdp.data_meta;
>>>> + offset = xdp.data - page_address(xdp_page) -
>>>> + vi->hdr_len - metasize;
>>>> /* recalculate len if xdp.data or xdp.data_end were
>>>> * adjusted
>>>> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>> if (unlikely(xdp_page != page)) {
>>>> rcu_read_unlock();
>>>> put_page(page);
>>>> - head_skb = page_to_skb(vi, rq, xdp_page,
>>>> - offset, len,
>>>> - PAGE_SIZE, false);
>>>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>>>> + len, PAGE_SIZE, false,
>>>> + metasize);
>>>> return head_skb;
>>>> }
>>>> break;
>>>> case XDP_TX:
>>>> stats->xdp_tx++;
>>>> + xdp.data_meta = xdp.data;
>>>> xdpf = convert_to_xdp_frame(&xdp);
>>>> if (unlikely(!xdpf))
>>>> goto err_xdp;
>>>> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>>>> goto err_skb;
>>>> }
>>>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>>>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>>>> + metasize);
>>>> curr_skb = head_skb;
>>>> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-09 3:04 ` Jason Wang
@ 2019-07-09 20:03 ` Daniel Borkmann
2019-07-10 2:30 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Borkmann @ 2019-07-09 20:03 UTC (permalink / raw)
To: Jason Wang, Yuya Kusakabe
Cc: ast, davem, hawk, jakub.kicinski, john.fastabend, kafai, mst,
netdev, songliubraving, yhs
On 07/09/2019 05:04 AM, Jason Wang wrote:
> On 2019/7/9 上午6:38, Daniel Borkmann wrote:
>> On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
>>> On 7/2/19 5:33 PM, Jason Wang wrote:
>>>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>>>>> This adds XDP meta data support to both receive_small() and
>>>>> receive_mergeable().
>>>>>
>>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>>>>> ---
>>>>> v3:
>>>>> - fix preserve the vnet header in receive_small().
>>>>> v2:
>>>>> - keep copy untouched in page_to_skb().
>>>>> - preserve the vnet header in receive_small().
>>>>> - fix indentation.
>>>>> ---
>>>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>>>>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> struct receive_queue *rq,
>>>>> struct page *page, unsigned int offset,
>>>>> unsigned int len, unsigned int truesize,
>>>>> - bool hdr_valid)
>>>>> + bool hdr_valid, unsigned int metasize)
>>>>> {
>>>>> struct sk_buff *skb;
>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> else
>>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>>> - if (hdr_valid)
>>>>> + if (hdr_valid && !metasize)
>>>>> memcpy(hdr, p, hdr_len);
>>>>> len -= hdr_len;
>>>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> copy = skb_tailroom(skb);
>>>>> skb_put_data(skb, p, copy);
>>>>> + if (metasize) {
>>>>> + __skb_pull(skb, metasize);
>>>>> + skb_metadata_set(skb, metasize);
>>>>> + }
>>>>> +
>>>>> len -= copy;
>>>>> offset += copy;
>>>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> unsigned int delta = 0;
>>>>> struct page *xdp_page;
>>>>> int err;
>>>>> + unsigned int metasize = 0;
>>>>> len -= vi->hdr_len;
>>>>> stats->bytes += len;
>>>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>>> - xdp_set_data_meta_invalid(&xdp);
>>>>> xdp.data_end = xdp.data + len;
>>>>> + xdp.data_meta = xdp.data;
>>>>> xdp.rxq = &rq->xdp_rxq;
>>>>> orig_data = xdp.data;
>>>>> + /* Copy the vnet header to the front of data_hard_start to avoid
>>>>> + * overwriting by XDP meta data */
>>>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>> I'm not fully sure if I'm following this one correctly, probably just missing
>> something. Isn't the vnet header based on how we set up xdp.data_hard_start
>> earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
>> vi->hdr_len into the vnet header at that point (given there can be up to 256
>> bytes of headroom between the two)? If it's relative to xdp.data and headroom
>> is >0, then BPF prog could otherwise mangle this; something doesn't add up to
>> me here. Could you clarify? Thx
>
> Vnet headr sits just in front of xdp.data not xdp.data_hard_start. So it could be overwrote by metadata, that's why we need a copy here.
For the current code, you can adjust the xdp.data with a positive/negative offset
already via bpf_xdp_adjust_head() helper. If vnet headr sits just in front of
xdp.data, couldn't this be overridden today as well then? Anyway, just wondering
how this is handled differently?
Thanks,
Daniel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-09 20:03 ` Daniel Borkmann
@ 2019-07-10 2:30 ` Jason Wang
2020-02-03 13:52 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2019-07-10 2:30 UTC (permalink / raw)
To: Daniel Borkmann, Yuya Kusakabe
Cc: ast, davem, hawk, jakub.kicinski, john.fastabend, kafai, mst,
netdev, songliubraving, yhs
On 2019/7/10 上午4:03, Daniel Borkmann wrote:
> On 07/09/2019 05:04 AM, Jason Wang wrote:
>> On 2019/7/9 上午6:38, Daniel Borkmann wrote:
>>> On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
>>>> On 7/2/19 5:33 PM, Jason Wang wrote:
>>>>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>>>>>> This adds XDP meta data support to both receive_small() and
>>>>>> receive_mergeable().
>>>>>>
>>>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>>>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>>>>>> ---
>>>>>> v3:
>>>>>> - fix preserve the vnet header in receive_small().
>>>>>> v2:
>>>>>> - keep copy untouched in page_to_skb().
>>>>>> - preserve the vnet header in receive_small().
>>>>>> - fix indentation.
>>>>>> ---
>>>>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>>>>>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>>>>>> --- a/drivers/net/virtio_net.c
>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>> struct receive_queue *rq,
>>>>>> struct page *page, unsigned int offset,
>>>>>> unsigned int len, unsigned int truesize,
>>>>>> - bool hdr_valid)
>>>>>> + bool hdr_valid, unsigned int metasize)
>>>>>> {
>>>>>> struct sk_buff *skb;
>>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>> else
>>>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>>>> - if (hdr_valid)
>>>>>> + if (hdr_valid && !metasize)
>>>>>> memcpy(hdr, p, hdr_len);
>>>>>> len -= hdr_len;
>>>>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>> copy = skb_tailroom(skb);
>>>>>> skb_put_data(skb, p, copy);
>>>>>> + if (metasize) {
>>>>>> + __skb_pull(skb, metasize);
>>>>>> + skb_metadata_set(skb, metasize);
>>>>>> + }
>>>>>> +
>>>>>> len -= copy;
>>>>>> offset += copy;
>>>>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>>> unsigned int delta = 0;
>>>>>> struct page *xdp_page;
>>>>>> int err;
>>>>>> + unsigned int metasize = 0;
>>>>>> len -= vi->hdr_len;
>>>>>> stats->bytes += len;
>>>>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>>>> - xdp_set_data_meta_invalid(&xdp);
>>>>>> xdp.data_end = xdp.data + len;
>>>>>> + xdp.data_meta = xdp.data;
>>>>>> xdp.rxq = &rq->xdp_rxq;
>>>>>> orig_data = xdp.data;
>>>>>> + /* Copy the vnet header to the front of data_hard_start to avoid
>>>>>> + * overwriting by XDP meta data */
>>>>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>>> I'm not fully sure if I'm following this one correctly, probably just missing
>>> something. Isn't the vnet header based on how we set up xdp.data_hard_start
>>> earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
>>> vi->hdr_len into the vnet header at that point (given there can be up to 256
>>> bytes of headroom between the two)? If it's relative to xdp.data and headroom
>>> is >0, then BPF prog could otherwise mangle this; something doesn't add up to
>>> me here. Could you clarify? Thx
>> Vnet headr sits just in front of xdp.data not xdp.data_hard_start. So it could be overwrote by metadata, that's why we need a copy here.
> For the current code, you can adjust the xdp.data with a positive/negative offset
> already via bpf_xdp_adjust_head() helper. If vnet headr sits just in front of
> xdp.data, couldn't this be overridden today as well then? Anyway, just wondering
> how this is handled differently?
We will invalidate the vnet header in this case. But for the case of
metadata adjustment without header adjustment, we want to seek a way to
preserve that.
Thanks
>
> Thanks,
> Daniel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2019-07-10 2:30 ` Jason Wang
@ 2020-02-03 13:52 ` Yuya Kusakabe
2020-02-04 3:31 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-03 13:52 UTC (permalink / raw)
To: Jason Wang
Cc: Daniel Borkmann, Alexei Starovoitov, davem,
Jesper Dangaard Brouer, Jakub Kicinski, John Fastabend,
Martin KaFai Lau, Michael S. Tsirkin, netdev, Song Liu,
Yonghong Song
I'm sorry for the late reply.
I saw the status of this patch is "Awaiting Upstream" on
https://patchwork.ozlabs.org/patch/1126046/. What is "Awaiting
Upstream"? Is there anything that I can do?
Thank you,
Yuya
On Wed, Jul 10, 2019 at 11:30 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/7/10 上午4:03, Daniel Borkmann wrote:
> > On 07/09/2019 05:04 AM, Jason Wang wrote:
> >> On 2019/7/9 上午6:38, Daniel Borkmann wrote:
> >>> On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
> >>>> On 7/2/19 5:33 PM, Jason Wang wrote:
> >>>>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
> >>>>>> This adds XDP meta data support to both receive_small() and
> >>>>>> receive_mergeable().
> >>>>>>
> >>>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> >>>>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> >>>>>> ---
> >>>>>> v3:
> >>>>>> - fix preserve the vnet header in receive_small().
> >>>>>> v2:
> >>>>>> - keep copy untouched in page_to_skb().
> >>>>>> - preserve the vnet header in receive_small().
> >>>>>> - fix indentation.
> >>>>>> ---
> >>>>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
> >>>>>> 1 file changed, 31 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
> >>>>>> --- a/drivers/net/virtio_net.c
> >>>>>> +++ b/drivers/net/virtio_net.c
> >>>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >>>>>> struct receive_queue *rq,
> >>>>>> struct page *page, unsigned int offset,
> >>>>>> unsigned int len, unsigned int truesize,
> >>>>>> - bool hdr_valid)
> >>>>>> + bool hdr_valid, unsigned int metasize)
> >>>>>> {
> >>>>>> struct sk_buff *skb;
> >>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
> >>>>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >>>>>> else
> >>>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
> >>>>>> - if (hdr_valid)
> >>>>>> + if (hdr_valid && !metasize)
> >>>>>> memcpy(hdr, p, hdr_len);
> >>>>>> len -= hdr_len;
> >>>>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >>>>>> copy = skb_tailroom(skb);
> >>>>>> skb_put_data(skb, p, copy);
> >>>>>> + if (metasize) {
> >>>>>> + __skb_pull(skb, metasize);
> >>>>>> + skb_metadata_set(skb, metasize);
> >>>>>> + }
> >>>>>> +
> >>>>>> len -= copy;
> >>>>>> offset += copy;
> >>>>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >>>>>> unsigned int delta = 0;
> >>>>>> struct page *xdp_page;
> >>>>>> int err;
> >>>>>> + unsigned int metasize = 0;
> >>>>>> len -= vi->hdr_len;
> >>>>>> stats->bytes += len;
> >>>>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >>>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> >>>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
> >>>>>> - xdp_set_data_meta_invalid(&xdp);
> >>>>>> xdp.data_end = xdp.data + len;
> >>>>>> + xdp.data_meta = xdp.data;
> >>>>>> xdp.rxq = &rq->xdp_rxq;
> >>>>>> orig_data = xdp.data;
> >>>>>> + /* Copy the vnet header to the front of data_hard_start to avoid
> >>>>>> + * overwriting by XDP meta data */
> >>>>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
> >>> I'm not fully sure if I'm following this one correctly, probably just missing
> >>> something. Isn't the vnet header based on how we set up xdp.data_hard_start
> >>> earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
> >>> vi->hdr_len into the vnet header at that point (given there can be up to 256
> >>> bytes of headroom between the two)? If it's relative to xdp.data and headroom
> >>> is >0, then BPF prog could otherwise mangle this; something doesn't add up to
> >>> me here. Could you clarify? Thx
> >> Vnet headr sits just in front of xdp.data not xdp.data_hard_start. So it could be overwrote by metadata, that's why we need a copy here.
> > For the current code, you can adjust the xdp.data with a positive/negative offset
> > already via bpf_xdp_adjust_head() helper. If vnet headr sits just in front of
> > xdp.data, couldn't this be overridden today as well then? Anyway, just wondering
> > how this is handled differently?
>
>
> We will invalidate the vnet header in this case. But for the case of
> metadata adjustment without header adjustment, we want to seek a way to
> preserve that.
>
> Thanks
>
>
> >
> > Thanks,
> > Daniel
--
Yuya Kusakabe
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
2020-02-03 13:52 ` Yuya Kusakabe
@ 2020-02-04 3:31 ` Jason Wang
2020-02-04 7:16 ` [PATCH bpf-next v4] " Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2020-02-04 3:31 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: Daniel Borkmann, Alexei Starovoitov, davem,
Jesper Dangaard Brouer, Jakub Kicinski, John Fastabend,
Martin KaFai Lau, Michael S. Tsirkin, netdev, Song Liu,
Yonghong Song
On 2020/2/3 下午9:52, Yuya Kusakabe wrote:
> I'm sorry for the late reply.
>
> I saw the status of this patch is "Awaiting Upstream" on
> https://patchwork.ozlabs.org/patch/1126046/. What is "Awaiting
> Upstream"? Is there anything that I can do?
You can post a new version I think.
Thanks
>
> Thank you,
> Yuya
>
> On Wed, Jul 10, 2019 at 11:30 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/7/10 上午4:03, Daniel Borkmann wrote:
>>> On 07/09/2019 05:04 AM, Jason Wang wrote:
>>>> On 2019/7/9 上午6:38, Daniel Borkmann wrote:
>>>>> On 07/02/2019 04:11 PM, Yuya Kusakabe wrote:
>>>>>> On 7/2/19 5:33 PM, Jason Wang wrote:
>>>>>>> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>>>>>>>> This adds XDP meta data support to both receive_small() and
>>>>>>>> receive_mergeable().
>>>>>>>>
>>>>>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>>>>>>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>>>>>>>> ---
>>>>>>>> v3:
>>>>>>>> - fix preserve the vnet header in receive_small().
>>>>>>>> v2:
>>>>>>>> - keep copy untouched in page_to_skb().
>>>>>>>> - preserve the vnet header in receive_small().
>>>>>>>> - fix indentation.
>>>>>>>> ---
>>>>>>>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>>>>>>>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>>>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>>>>>>>> --- a/drivers/net/virtio_net.c
>>>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>>>> struct receive_queue *rq,
>>>>>>>> struct page *page, unsigned int offset,
>>>>>>>> unsigned int len, unsigned int truesize,
>>>>>>>> - bool hdr_valid)
>>>>>>>> + bool hdr_valid, unsigned int metasize)
>>>>>>>> {
>>>>>>>> struct sk_buff *skb;
>>>>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>>>>>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>>>> else
>>>>>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>>>>>> - if (hdr_valid)
>>>>>>>> + if (hdr_valid && !metasize)
>>>>>>>> memcpy(hdr, p, hdr_len);
>>>>>>>> len -= hdr_len;
>>>>>>>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>>>>> copy = skb_tailroom(skb);
>>>>>>>> skb_put_data(skb, p, copy);
>>>>>>>> + if (metasize) {
>>>>>>>> + __skb_pull(skb, metasize);
>>>>>>>> + skb_metadata_set(skb, metasize);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> len -= copy;
>>>>>>>> offset += copy;
>>>>>>>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>>>>> unsigned int delta = 0;
>>>>>>>> struct page *xdp_page;
>>>>>>>> int err;
>>>>>>>> + unsigned int metasize = 0;
>>>>>>>> len -= vi->hdr_len;
>>>>>>>> stats->bytes += len;
>>>>>>>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>>>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>>>>>> - xdp_set_data_meta_invalid(&xdp);
>>>>>>>> xdp.data_end = xdp.data + len;
>>>>>>>> + xdp.data_meta = xdp.data;
>>>>>>>> xdp.rxq = &rq->xdp_rxq;
>>>>>>>> orig_data = xdp.data;
>>>>>>>> + /* Copy the vnet header to the front of data_hard_start to avoid
>>>>>>>> + * overwriting by XDP meta data */
>>>>>>>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>>>>> I'm not fully sure if I'm following this one correctly, probably just missing
>>>>> something. Isn't the vnet header based on how we set up xdp.data_hard_start
>>>>> earlier already in front of it? Wouldn't we copy invalid data from xdp.data -
>>>>> vi->hdr_len into the vnet header at that point (given there can be up to 256
>>>>> bytes of headroom between the two)? If it's relative to xdp.data and headroom
>>>>> is >0, then BPF prog could otherwise mangle this; something doesn't add up to
>>>>> me here. Could you clarify? Thx
>>>> Vnet headr sits just in front of xdp.data not xdp.data_hard_start. So it could be overwrote by metadata, that's why we need a copy here.
>>> For the current code, you can adjust the xdp.data with a positive/negative offset
>>> already via bpf_xdp_adjust_head() helper. If vnet headr sits just in front of
>>> xdp.data, couldn't this be overridden today as well then? Anyway, just wondering
>>> how this is handled differently?
>>
>> We will invalidate the vnet header in this case. But for the case of
>> metadata adjustment without header adjustment, we want to seek a way to
>> preserve that.
>>
>> Thanks
>>
>>
>>> Thanks,
>>> Daniel
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-04 3:31 ` Jason Wang
@ 2020-02-04 7:16 ` Yuya Kusakabe
2020-02-05 4:10 ` Jason Wang
2020-02-05 5:33 ` [PATCH bpf-next v4] " Michael S. Tsirkin
0 siblings, 2 replies; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-04 7:16 UTC (permalink / raw)
To: jasowang
Cc: ast, daniel, davem, hawk, john.fastabend, kafai, mst,
songliubraving, yhs, kuba, andriin, yuya.kusakabe, netdev,
linux-kernel, bpf
Implement support for transferring XDP meta data into skb for
virtio_net driver; before calling into the program, xdp.data_meta points
to xdp.data and copy vnet header to the front of xdp.data_hard_start
to avoid overwriting it, where on program return with pass verdict,
we call into skb_metadata_set().
Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
---
drivers/net/virtio_net.c | 47 ++++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a3188282..5fdd6ea0e3f1 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize,
- bool hdr_valid)
+ bool hdr_valid, unsigned int metasize)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
- if (hdr_valid)
+ if (hdr_valid && !metasize)
memcpy(hdr, p, hdr_len);
len -= hdr_len;
@@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
copy = skb_tailroom(skb);
skb_put_data(skb, p, copy);
+ if (metasize) {
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ }
+
len -= copy;
offset += copy;
@@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int delta = 0;
struct page *xdp_page;
int err;
+ unsigned int metasize = 0;
len -= vi->hdr_len;
stats->bytes += len;
@@ -683,10 +689,15 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
+ /* Copy the vnet header to the front of data_hard_start to avoid
+ * overwriting it by XDP meta data.
+ */
+ memcpy(xdp.data_hard_start - vi->hdr_len,
+ xdp.data - vi->hdr_len, vi->hdr_len);
act = bpf_prog_run_xdp(xdp_prog, &xdp);
stats->xdp_packets++;
@@ -695,9 +706,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
/* Recalculate length in case bpf program changed it */
delta = orig_data - xdp.data;
len = xdp.data_end - xdp.data;
+ metasize = xdp.data - xdp.data_meta;
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -736,10 +749,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
skb_reserve(skb, headroom - delta);
skb_put(skb, len);
if (!delta) {
- buf += header_offset;
- memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
+ memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
+ if (metasize)
+ skb_metadata_set(skb, metasize);
+
err:
return skb;
@@ -760,8 +775,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
- PAGE_SIZE, true);
+ struct sk_buff *skb =
+ page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -793,6 +808,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
int err;
+ unsigned int metasize = 0;
head_skb = NULL;
stats->bytes += len - vi->hdr_len;
@@ -839,8 +855,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
data = page_address(xdp_page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + (len - vi->hdr_len);
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -852,8 +868,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
* adjustments. Note other cases do not build an
* skb and avoid using offset
*/
- offset = xdp.data -
- page_address(xdp_page) - vi->hdr_len;
+ metasize = xdp.data - xdp.data_meta;
+ offset = xdp.data - page_address(xdp_page) -
+ vi->hdr_len - metasize;
/* recalculate len if xdp.data or xdp.data_end were
* adjusted
@@ -863,14 +880,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
if (unlikely(xdp_page != page)) {
rcu_read_unlock();
put_page(page);
- head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len,
- PAGE_SIZE, false);
+ head_skb = page_to_skb(vi, rq, xdp_page, offset,
+ len, PAGE_SIZE, false,
+ metasize);
return head_skb;
}
break;
case XDP_TX:
stats->xdp_tx++;
+ xdp.data_meta = xdp.data;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -921,7 +939,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
+ metasize);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.24.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-04 7:16 ` [PATCH bpf-next v4] " Yuya Kusakabe
@ 2020-02-05 4:10 ` Jason Wang
2020-02-05 9:18 ` Yuya Kusakabe
2020-02-05 5:33 ` [PATCH bpf-next v4] " Michael S. Tsirkin
1 sibling, 1 reply; 28+ messages in thread
From: Jason Wang @ 2020-02-05 4:10 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: ast, daniel, davem, hawk, john.fastabend, kafai, mst,
songliubraving, yhs, kuba, andriin, netdev, linux-kernel, bpf
On 2020/2/4 下午3:16, Yuya Kusakabe wrote:
> Implement support for transferring XDP meta data into skb for
> virtio_net driver; before calling into the program, xdp.data_meta points
> to xdp.data and copy vnet header to the front of xdp.data_hard_start
> to avoid overwriting it, where on program return with pass verdict,
> we call into skb_metadata_set().
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> ---
> drivers/net/virtio_net.c | 47 ++++++++++++++++++++++++++++------------
> 1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2fe7a3188282..5fdd6ea0e3f1 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> struct page *page, unsigned int offset,
> unsigned int len, unsigned int truesize,
> - bool hdr_valid)
> + bool hdr_valid, unsigned int metasize)
> {
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> - if (hdr_valid)
> + if (hdr_valid && !metasize)
hdr_valid means no XDP, so I think we can remove the check for metasize
here and add a comment instead?
> memcpy(hdr, p, hdr_len);
>
> len -= hdr_len;
> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> copy = skb_tailroom(skb);
> skb_put_data(skb, p, copy);
>
> + if (metasize) {
> + __skb_pull(skb, metasize);
> + skb_metadata_set(skb, metasize);
> + }
> +
> len -= copy;
> offset += copy;
>
> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> unsigned int delta = 0;
> struct page *xdp_page;
> int err;
> + unsigned int metasize = 0;
>
> len -= vi->hdr_len;
> stats->bytes += len;
> @@ -683,10 +689,15 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + len;
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> + /* Copy the vnet header to the front of data_hard_start to avoid
> + * overwriting it by XDP meta data.
> + */
> + memcpy(xdp.data_hard_start - vi->hdr_len,
> + xdp.data - vi->hdr_len, vi->hdr_len);
I think we don't need this. And it looks to me there's a bug in the
current code.
Commit 436c9453a1ac0 ("virtio-net: keep vnet header zeroed after
processing XDP") leave the a corner case for receive_small() which still
use:
if (!delta) {
buf += header_offset;
memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
Which seems wrong, we need check xdp_prog instead of delta.
With this fixed, there's no need to care about the vnet header here
since we don't know whether or not packet is modified by XDP.
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> stats->xdp_packets++;
>
> @@ -695,9 +706,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
> /* Recalculate length in case bpf program changed it */
> delta = orig_data - xdp.data;
> len = xdp.data_end - xdp.data;
> + metasize = xdp.data - xdp.data_meta;
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
I think we should remove the xdp_set_data_meta_invalid() at least? And
move this initialization just after xdp.data is initialized.
Testing receive_small() requires to disable mrg_rxbuf, guest_tso4,
guest_tso6 and guest_ufo from qemu command line.
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -736,10 +749,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
> skb_reserve(skb, headroom - delta);
> skb_put(skb, len);
> if (!delta) {
Need to check xdp_prog (need another patch).
> - buf += header_offset;
> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> err:
> return skb;
>
> @@ -760,8 +775,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> - PAGE_SIZE, true);
> + struct sk_buff *skb =
> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>
> stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> @@ -793,6 +808,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> unsigned int truesize;
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> int err;
> + unsigned int metasize = 0;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> @@ -839,8 +855,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> data = page_address(xdp_page) + offset;
> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> xdp.data = data + vi->hdr_len;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + (len - vi->hdr_len);
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -852,8 +868,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> * adjustments. Note other cases do not build an
> * skb and avoid using offset
> */
> - offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> + metasize = xdp.data - xdp.data_meta;
> + offset = xdp.data - page_address(xdp_page) -
> + vi->hdr_len - metasize;
>
> /* recalculate len if xdp.data or xdp.data_end were
> * adjusted
> @@ -863,14 +880,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> if (unlikely(xdp_page != page)) {
> rcu_read_unlock();
> put_page(page);
> - head_skb = page_to_skb(vi, rq, xdp_page,
> - offset, len,
> - PAGE_SIZE, false);
> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> + len, PAGE_SIZE, false,
> + metasize);
> return head_skb;
> }
> break;
> case XDP_TX:
> stats->xdp_tx++;
> + xdp.data_meta = xdp.data;
Any reason for doing this?
Thanks
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -921,7 +939,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_skb;
> }
>
> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> + metasize);
> curr_skb = head_skb;
>
> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-04 7:16 ` [PATCH bpf-next v4] " Yuya Kusakabe
2020-02-05 4:10 ` Jason Wang
@ 2020-02-05 5:33 ` Michael S. Tsirkin
2020-02-05 9:19 ` Yuya Kusakabe
1 sibling, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-02-05 5:33 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: jasowang, ast, daniel, davem, hawk, john.fastabend, kafai,
songliubraving, yhs, kuba, andriin, netdev, linux-kernel, bpf
On Tue, Feb 04, 2020 at 04:16:55PM +0900, Yuya Kusakabe wrote:
> @@ -852,8 +868,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> * adjustments. Note other cases do not build an
> * skb and avoid using offset
> */
> - offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> + metasize = xdp.data - xdp.data_meta;
> + offset = xdp.data - page_address(xdp_page) -
> + vi->hdr_len - metasize;
>
> /* recalculate len if xdp.data or xdp.data_end were
> * adjusted
Tricky to get one's head around.
Can you pls update the comment above to document the new math?
--
MST
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-05 4:10 ` Jason Wang
@ 2020-02-05 9:18 ` Yuya Kusakabe
2020-02-06 3:20 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-05 9:18 UTC (permalink / raw)
To: Jason Wang
Cc: ast, daniel, davem, hawk, john.fastabend, kafai, mst,
songliubraving, yhs, kuba, andriin, netdev, linux-kernel, bpf
On 2/5/20 1:10 PM, Jason Wang wrote:
>
> On 2020/2/4 下午3:16, Yuya Kusakabe wrote:
>> Implement support for transferring XDP meta data into skb for
>> virtio_net driver; before calling into the program, xdp.data_meta points
>> to xdp.data and copy vnet header to the front of xdp.data_hard_start
>> to avoid overwriting it, where on program return with pass verdict,
>> we call into skb_metadata_set().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> drivers/net/virtio_net.c | 47 ++++++++++++++++++++++++++++------------
>> 1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 2fe7a3188282..5fdd6ea0e3f1 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>
>
> hdr_valid means no XDP, so I think we can remove the check for metasize here and add a comment instead?
I will fix it on next patch.
>
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,10 +689,15 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> + /* Copy the vnet header to the front of data_hard_start to avoid
>> + * overwriting it by XDP meta data.
>> + */
>> + memcpy(xdp.data_hard_start - vi->hdr_len,
>> + xdp.data - vi->hdr_len, vi->hdr_len);
>
>
> I think we don't need this. And it looks to me there's a bug in the current code.
>
> Commit 436c9453a1ac0 ("virtio-net: keep vnet header zeroed after processing XDP") leave the a corner case for receive_small() which still use:
>
> if (!delta) {
> buf += header_offset;
> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
> Which seems wrong, we need check xdp_prog instead of delta.
>
> With this fixed, there's no need to care about the vnet header here since we don't know whether or not packet is modified by XDP.
I missed this commit. I understand this is the reason for "Awaiting Upstream".
>
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> stats->xdp_packets++;
>> @@ -695,9 +706,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>
>
> I think we should remove the xdp_set_data_meta_invalid() at least? And move this initialization just after xdp.data is initialized.
>
> Testing receive_small() requires to disable mrg_rxbuf, guest_tso4, guest_tso6 and guest_ufo from qemu command line.
>
>
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -736,10 +749,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> if (!delta) {
>
>
> Need to check xdp_prog (need another patch).
I will fix it on next patch.
>
>
>> - buf += header_offset;
>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +775,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +808,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +855,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -852,8 +868,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>> @@ -863,14 +880,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>
>
> Any reason for doing this?
XDP_TX can not support metadata for now, because if metasize > 0, __virtnet_xdp_xmit_one() returns EOPNOTSUPP.
static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
struct send_queue *sq,
struct xdp_frame *xdpf)
{
struct virtio_net_hdr_mrg_rxbuf *hdr;
int err;
/* virtqueue want to use data area in-front of packet */
if (unlikely(xdpf->metasize > 0))
return -EOPNOTSUPP;
>
> Thanks
>
>
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +939,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
>
Thank you for your kind review.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-05 5:33 ` [PATCH bpf-next v4] " Michael S. Tsirkin
@ 2020-02-05 9:19 ` Yuya Kusakabe
0 siblings, 0 replies; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-05 9:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, ast, daniel, davem, hawk, john.fastabend, kafai,
songliubraving, yhs, kuba, andriin, netdev, linux-kernel, bpf
On 2/5/20 2:33 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 04, 2020 at 04:16:55PM +0900, Yuya Kusakabe wrote:
>> @@ -852,8 +868,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>>
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>
> Tricky to get one's head around.
> Can you pls update the comment above to document the new math?
>
Thank you for your review.
I will update the comment on next patch.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v4] virtio_net: add XDP meta data support
2020-02-05 9:18 ` Yuya Kusakabe
@ 2020-02-06 3:20 ` Jason Wang
2020-02-20 8:55 ` [PATCH bpf-next v5] " Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2020-02-06 3:20 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: ast, daniel, davem, hawk, john.fastabend, kafai, mst,
songliubraving, yhs, kuba, andriin, netdev, linux-kernel, bpf
On 2020/2/5 下午5:18, Yuya Kusakabe wrote:
>>> case XDP_TX:
>>> stats->xdp_tx++;
>>> + xdp.data_meta = xdp.data;
>> Any reason for doing this?
> XDP_TX can not support metadata for now, because if metasize > 0, __virtnet_xdp_xmit_one() returns EOPNOTSUPP.
>
> static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
> struct send_queue *sq,
> struct xdp_frame *xdpf)
> {
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> int err;
>
> /* virtqueue want to use data area in-front of packet */
> if (unlikely(xdpf->metasize > 0))
> return -EOPNOTSUPP;
>
>
I see.
Then I think it's better to fix __virtnet_xdp_xmit_one() instead.
Thanks
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-06 3:20 ` Jason Wang
@ 2020-02-20 8:55 ` Yuya Kusakabe
2020-02-21 4:23 ` Jason Wang
0 siblings, 1 reply; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-20 8:55 UTC (permalink / raw)
To: jasowang
Cc: andriin, ast, bpf, daniel, davem, hawk, john.fastabend, kafai,
kuba, linux-kernel, mst, netdev, songliubraving, yhs,
yuya.kusakabe
Implement support for transferring XDP meta data into skb for
virtio_net driver; before calling into the program, xdp.data_meta points
to xdp.data, where on program return with pass verdict, we call
into skb_metadata_set().
Tested with the script at
https://github.com/higebu/virtio_net-xdp-metadata-test.
Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
---
v5:
- page_to_skb(): copy vnet header if hdr_valid without checking metasize.
- receive_small(): do not copy vnet header if xdp_prog is availavle.
- __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
- improve comments.
v4:
- improve commit message
v3:
- fix preserve the vnet header in receive_small().
v2:
- keep copy untouched in page_to_skb().
- preserve the vnet header in receive_small().
- fix indentation.
---
drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 21 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a3188282..4ea0ae60c000 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
struct page *page, unsigned int offset,
unsigned int len, unsigned int truesize,
- bool hdr_valid)
+ bool hdr_valid, unsigned int metasize)
{
struct sk_buff *skb;
struct virtio_net_hdr_mrg_rxbuf *hdr;
@@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
else
hdr_padded_len = sizeof(struct padded_vnet_hdr);
+ /* hdr_valid means no XDP, so we can copy the vnet header */
if (hdr_valid)
memcpy(hdr, p, hdr_len);
@@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
copy = skb_tailroom(skb);
skb_put_data(skb, p, copy);
+ if (metasize) {
+ __skb_pull(skb, metasize);
+ skb_metadata_set(skb, metasize);
+ }
+
len -= copy;
offset += copy;
@@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
struct virtio_net_hdr_mrg_rxbuf *hdr;
int err;
- /* virtqueue want to use data area in-front of packet */
- if (unlikely(xdpf->metasize > 0))
- return -EOPNOTSUPP;
-
if (unlikely(xdpf->headroom < vi->hdr_len))
return -EOVERFLOW;
@@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
unsigned int delta = 0;
struct page *xdp_page;
int err;
+ unsigned int metasize = 0;
len -= vi->hdr_len;
stats->bytes += len;
@@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
/* Recalculate length in case bpf program changed it */
delta = orig_data - xdp.data;
len = xdp.data_end - xdp.data;
+ metasize = xdp.data - xdp.data_meta;
break;
case XDP_TX:
stats->xdp_tx++;
@@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
}
skb_reserve(skb, headroom - delta);
skb_put(skb, len);
- if (!delta) {
+ if (!xdp_prog) {
buf += header_offset;
memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
} /* keep zeroed vnet hdr since packet was changed by bpf */
+ if (metasize)
+ skb_metadata_set(skb, metasize);
+
err:
return skb;
@@ -760,8 +767,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct virtnet_rq_stats *stats)
{
struct page *page = buf;
- struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
- PAGE_SIZE, true);
+ struct sk_buff *skb =
+ page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
@@ -793,6 +800,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
unsigned int truesize;
unsigned int headroom = mergeable_ctx_to_headroom(ctx);
int err;
+ unsigned int metasize = 0;
head_skb = NULL;
stats->bytes += len - vi->hdr_len;
@@ -839,8 +847,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
data = page_address(xdp_page) + offset;
xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
xdp.data = data + vi->hdr_len;
- xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + (len - vi->hdr_len);
+ xdp.data_meta = xdp.data;
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -848,24 +856,27 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
switch (act) {
case XDP_PASS:
+ metasize = xdp.data - xdp.data_meta;
+
/* recalculate offset to account for any header
- * adjustments. Note other cases do not build an
- * skb and avoid using offset
+ * adjustments and minus the metasize to copy the
+ * metadata in page_to_skb(). Note other cases do not
+ * build an skb and avoid using offset
*/
- offset = xdp.data -
- page_address(xdp_page) - vi->hdr_len;
+ offset = xdp.data - page_address(xdp_page) -
+ vi->hdr_len - metasize;
- /* recalculate len if xdp.data or xdp.data_end were
- * adjusted
+ /* recalculate len if xdp.data, xdp.data_end or
+ * xdp.data_meta were adjusted
*/
- len = xdp.data_end - xdp.data + vi->hdr_len;
+ len = xdp.data_end - xdp.data + vi->hdr_len + metasize;
/* We can only create skb based on xdp_page. */
if (unlikely(xdp_page != page)) {
rcu_read_unlock();
put_page(page);
- head_skb = page_to_skb(vi, rq, xdp_page,
- offset, len,
- PAGE_SIZE, false);
+ head_skb = page_to_skb(vi, rq, xdp_page, offset,
+ len, PAGE_SIZE, false,
+ metasize);
return head_skb;
}
break;
@@ -921,7 +932,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_skb;
}
- head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
+ head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
+ metasize);
curr_skb = head_skb;
if (unlikely(!curr_skb))
--
2.24.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-20 8:55 ` [PATCH bpf-next v5] " Yuya Kusakabe
@ 2020-02-21 4:23 ` Jason Wang
2020-02-21 8:36 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2020-02-21 4:23 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: andriin, ast, bpf, daniel, davem, hawk, john.fastabend, kafai,
kuba, linux-kernel, mst, netdev, songliubraving, yhs
On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
> Implement support for transferring XDP meta data into skb for
> virtio_net driver; before calling into the program, xdp.data_meta points
> to xdp.data, where on program return with pass verdict, we call
> into skb_metadata_set().
>
> Tested with the script at
> https://github.com/higebu/virtio_net-xdp-metadata-test.
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
I'm not sure this is correct since virtio-net claims to not support
metadata by calling xdp_set_data_meta_invalid()?
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> ---
> v5:
> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
> - receive_small(): do not copy vnet header if xdp_prog is availavle.
> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
> - improve comments.
> v4:
> - improve commit message
> v3:
> - fix preserve the vnet header in receive_small().
> v2:
> - keep copy untouched in page_to_skb().
> - preserve the vnet header in receive_small().
> - fix indentation.
> ---
> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2fe7a3188282..4ea0ae60c000 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> struct page *page, unsigned int offset,
> unsigned int len, unsigned int truesize,
> - bool hdr_valid)
> + bool hdr_valid, unsigned int metasize)
> {
> struct sk_buff *skb;
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> else
> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>
> + /* hdr_valid means no XDP, so we can copy the vnet header */
> if (hdr_valid)
> memcpy(hdr, p, hdr_len);
>
> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> copy = skb_tailroom(skb);
> skb_put_data(skb, p, copy);
>
> + if (metasize) {
> + __skb_pull(skb, metasize);
> + skb_metadata_set(skb, metasize);
> + }
> +
> len -= copy;
> offset += copy;
>
> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
> struct virtio_net_hdr_mrg_rxbuf *hdr;
> int err;
>
> - /* virtqueue want to use data area in-front of packet */
> - if (unlikely(xdpf->metasize > 0))
> - return -EOPNOTSUPP;
> -
> if (unlikely(xdpf->headroom < vi->hdr_len))
> return -EOVERFLOW;
>
> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> unsigned int delta = 0;
> struct page *xdp_page;
> int err;
> + unsigned int metasize = 0;
>
> len -= vi->hdr_len;
> stats->bytes += len;
> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + len;
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> /* Recalculate length in case bpf program changed it */
> delta = orig_data - xdp.data;
> len = xdp.data_end - xdp.data;
> + metasize = xdp.data - xdp.data_meta;
> break;
> case XDP_TX:
> stats->xdp_tx++;
> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
> }
> skb_reserve(skb, headroom - delta);
> skb_put(skb, len);
> - if (!delta) {
> + if (!xdp_prog) {
> buf += header_offset;
> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> } /* keep zeroed vnet hdr since packet was changed by bpf */
I prefer to make this an independent patch and cc stable.
Other looks good.
Thanks
>
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> err:
> return skb;
>
> @@ -760,8 +767,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> - PAGE_SIZE, true);
> + struct sk_buff *skb =
> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>
> stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> @@ -793,6 +800,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> unsigned int truesize;
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> int err;
> + unsigned int metasize = 0;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> @@ -839,8 +847,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> data = page_address(xdp_page) + offset;
> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> xdp.data = data + vi->hdr_len;
> - xdp_set_data_meta_invalid(&xdp);
> xdp.data_end = xdp.data + (len - vi->hdr_len);
> + xdp.data_meta = xdp.data;
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -848,24 +856,27 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>
> switch (act) {
> case XDP_PASS:
> + metasize = xdp.data - xdp.data_meta;
> +
> /* recalculate offset to account for any header
> - * adjustments. Note other cases do not build an
> - * skb and avoid using offset
> + * adjustments and minus the metasize to copy the
> + * metadata in page_to_skb(). Note other cases do not
> + * build an skb and avoid using offset
> */
> - offset = xdp.data -
> - page_address(xdp_page) - vi->hdr_len;
> + offset = xdp.data - page_address(xdp_page) -
> + vi->hdr_len - metasize;
>
> - /* recalculate len if xdp.data or xdp.data_end were
> - * adjusted
> + /* recalculate len if xdp.data, xdp.data_end or
> + * xdp.data_meta were adjusted
> */
> - len = xdp.data_end - xdp.data + vi->hdr_len;
> + len = xdp.data_end - xdp.data + vi->hdr_len + metasize;
> /* We can only create skb based on xdp_page. */
> if (unlikely(xdp_page != page)) {
> rcu_read_unlock();
> put_page(page);
> - head_skb = page_to_skb(vi, rq, xdp_page,
> - offset, len,
> - PAGE_SIZE, false);
> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> + len, PAGE_SIZE, false,
> + metasize);
> return head_skb;
> }
> break;
> @@ -921,7 +932,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_skb;
> }
>
> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> + metasize);
> curr_skb = head_skb;
>
> if (unlikely(!curr_skb))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-21 4:23 ` Jason Wang
@ 2020-02-21 8:36 ` Yuya Kusakabe
2020-02-21 11:01 ` Michael S. Tsirkin
2020-02-23 8:14 ` Michael S. Tsirkin
0 siblings, 2 replies; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-21 8:36 UTC (permalink / raw)
To: Jason Wang
Cc: andriin, ast, bpf, daniel, davem, hawk, john.fastabend, kafai,
kuba, linux-kernel, mst, netdev, songliubraving, yhs
On 2/21/20 1:23 PM, Jason Wang wrote:
>
> On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
>> Implement support for transferring XDP meta data into skb for
>> virtio_net driver; before calling into the program, xdp.data_meta points
>> to xdp.data, where on program return with pass verdict, we call
>> into skb_metadata_set().
>>
>> Tested with the script at
>> https://github.com/higebu/virtio_net-xdp-metadata-test.
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>
>
> I'm not sure this is correct since virtio-net claims to not support metadata by calling xdp_set_data_meta_invalid()?
virtio_net doesn't support by calling xdp_set_data_meta_invalid() for now.
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n686
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n842
And xdp_set_data_meta_invalid() are added by de8f3a83b0a0.
$ git blame ./drivers/net/virtio_net.c | grep xdp_set_data_meta_invalid
de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 686) xdp_set_data_meta_invalid(&xdp);
de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 842) xdp_set_data_meta_invalid(&xdp);
So I added `Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")` to the comment.
>
>
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> v5:
>> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
>> - receive_small(): do not copy vnet header if xdp_prog is availavle.
>> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
>> - improve comments.
>> v4:
>> - improve commit message
>> v3:
>> - fix preserve the vnet header in receive_small().
>> v2:
>> - keep copy untouched in page_to_skb().
>> - preserve the vnet header in receive_small().
>> - fix indentation.
>> ---
>> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
>> 1 file changed, 33 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 2fe7a3188282..4ea0ae60c000 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> + /* hdr_valid means no XDP, so we can copy the vnet header */
>> if (hdr_valid)
>> memcpy(hdr, p, hdr_len);
>> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> int err;
>> - /* virtqueue want to use data area in-front of packet */
>> - if (unlikely(xdpf->metasize > 0))
>> - return -EOPNOTSUPP;
>> -
>> if (unlikely(xdpf->headroom < vi->hdr_len))
>> return -EOVERFLOW;
>> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> }
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> - if (!delta) {
>> + if (!xdp_prog) {
>> buf += header_offset;
>> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>
>
> I prefer to make this an independent patch and cc stable.
>
> Other looks good.
>
> Thanks
I see. So I need to revert to delta from xdp_prog?
Thank you.
>
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +767,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +800,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +847,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -848,24 +856,27 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> switch (act) {
>> case XDP_PASS:
>> + metasize = xdp.data - xdp.data_meta;
>> +
>> /* recalculate offset to account for any header
>> - * adjustments. Note other cases do not build an
>> - * skb and avoid using offset
>> + * adjustments and minus the metasize to copy the
>> + * metadata in page_to_skb(). Note other cases do not
>> + * build an skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> - /* recalculate len if xdp.data or xdp.data_end were
>> - * adjusted
>> + /* recalculate len if xdp.data, xdp.data_end or
>> + * xdp.data_meta were adjusted
>> */
>> - len = xdp.data_end - xdp.data + vi->hdr_len;
>> + len = xdp.data_end - xdp.data + vi->hdr_len + metasize;
>> /* We can only create skb based on xdp_page. */
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> @@ -921,7 +932,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-21 8:36 ` Yuya Kusakabe
@ 2020-02-21 11:01 ` Michael S. Tsirkin
2020-02-23 8:14 ` Michael S. Tsirkin
1 sibling, 0 replies; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-02-21 11:01 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: Jason Wang, andriin, ast, bpf, daniel, davem, hawk,
john.fastabend, kafai, kuba, linux-kernel, netdev,
songliubraving, yhs
On Fri, Feb 21, 2020 at 05:36:08PM +0900, Yuya Kusakabe wrote:
> On 2/21/20 1:23 PM, Jason Wang wrote:
> >
> > On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
> >> Implement support for transferring XDP meta data into skb for
> >> virtio_net driver; before calling into the program, xdp.data_meta points
> >> to xdp.data, where on program return with pass verdict, we call
> >> into skb_metadata_set().
> >>
> >> Tested with the script at
> >> https://github.com/higebu/virtio_net-xdp-metadata-test.
> >>
> >> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> >
> >
> > I'm not sure this is correct since virtio-net claims to not support metadata by calling xdp_set_data_meta_invalid()?
>
> virtio_net doesn't support by calling xdp_set_data_meta_invalid() for now.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n686
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n842
>
> And xdp_set_data_meta_invalid() are added by de8f3a83b0a0.
>
> $ git blame ./drivers/net/virtio_net.c | grep xdp_set_data_meta_invalid
> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 686) xdp_set_data_meta_invalid(&xdp);
> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 842) xdp_set_data_meta_invalid(&xdp);
>
> So I added `Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")` to the comment.
Fixes basically means "must be backported to any kernel that has
de8f3a83b0a0 in order to fix a bug". This looks more like
a feature than a bug though, so I'm not sure Fixes
is approproate. Correct me if I'm wrong.
> >
> >
> >> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> >> ---
> >> v5:
> >> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
> >> - receive_small(): do not copy vnet header if xdp_prog is availavle.
> >> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
> >> - improve comments.
> >> v4:
> >> - improve commit message
> >> v3:
> >> - fix preserve the vnet header in receive_small().
> >> v2:
> >> - keep copy untouched in page_to_skb().
> >> - preserve the vnet header in receive_small().
> >> - fix indentation.
> >> ---
> >> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
> >> 1 file changed, 33 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 2fe7a3188282..4ea0ae60c000 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> struct receive_queue *rq,
> >> struct page *page, unsigned int offset,
> >> unsigned int len, unsigned int truesize,
> >> - bool hdr_valid)
> >> + bool hdr_valid, unsigned int metasize)
> >> {
> >> struct sk_buff *skb;
> >> struct virtio_net_hdr_mrg_rxbuf *hdr;
> >> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> else
> >> hdr_padded_len = sizeof(struct padded_vnet_hdr);
> >> + /* hdr_valid means no XDP, so we can copy the vnet header */
> >> if (hdr_valid)
> >> memcpy(hdr, p, hdr_len);
> >> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> copy = skb_tailroom(skb);
> >> skb_put_data(skb, p, copy);
> >> + if (metasize) {
> >> + __skb_pull(skb, metasize);
> >> + skb_metadata_set(skb, metasize);
> >> + }
> >> +
> >> len -= copy;
> >> offset += copy;
> >> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
> >> struct virtio_net_hdr_mrg_rxbuf *hdr;
> >> int err;
> >> - /* virtqueue want to use data area in-front of packet */
> >> - if (unlikely(xdpf->metasize > 0))
> >> - return -EOPNOTSUPP;
> >> -
> >> if (unlikely(xdpf->headroom < vi->hdr_len))
> >> return -EOVERFLOW;
> >> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> unsigned int delta = 0;
> >> struct page *xdp_page;
> >> int err;
> >> + unsigned int metasize = 0;
> >> len -= vi->hdr_len;
> >> stats->bytes += len;
> >> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> >> xdp.data = xdp.data_hard_start + xdp_headroom;
> >> - xdp_set_data_meta_invalid(&xdp);
> >> xdp.data_end = xdp.data + len;
> >> + xdp.data_meta = xdp.data;
> >> xdp.rxq = &rq->xdp_rxq;
> >> orig_data = xdp.data;
> >> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> /* Recalculate length in case bpf program changed it */
> >> delta = orig_data - xdp.data;
> >> len = xdp.data_end - xdp.data;
> >> + metasize = xdp.data - xdp.data_meta;
> >> break;
> >> case XDP_TX:
> >> stats->xdp_tx++;
> >> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> }
> >> skb_reserve(skb, headroom - delta);
> >> skb_put(skb, len);
> >> - if (!delta) {
> >> + if (!xdp_prog) {
> >> buf += header_offset;
> >> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> >> } /* keep zeroed vnet hdr since packet was changed by bpf */
> >
> >
> > I prefer to make this an independent patch and cc stable.
> >
> > Other looks good.
> >
> > Thanks
>
> I see. So I need to revert to delta from xdp_prog?
>
> Thank you.
>
> >
> >> + if (metasize)
> >> + skb_metadata_set(skb, metasize);
> >> +
> >> err:
> >> return skb;
> >> @@ -760,8 +767,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> >> struct virtnet_rq_stats *stats)
> >> {
> >> struct page *page = buf;
> >> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> >> - PAGE_SIZE, true);
> >> + struct sk_buff *skb =
> >> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
> >> stats->bytes += len - vi->hdr_len;
> >> if (unlikely(!skb))
> >> @@ -793,6 +800,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> unsigned int truesize;
> >> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> >> int err;
> >> + unsigned int metasize = 0;
> >> head_skb = NULL;
> >> stats->bytes += len - vi->hdr_len;
> >> @@ -839,8 +847,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> data = page_address(xdp_page) + offset;
> >> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> >> xdp.data = data + vi->hdr_len;
> >> - xdp_set_data_meta_invalid(&xdp);
> >> xdp.data_end = xdp.data + (len - vi->hdr_len);
> >> + xdp.data_meta = xdp.data;
> >> xdp.rxq = &rq->xdp_rxq;
> >> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> @@ -848,24 +856,27 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> switch (act) {
> >> case XDP_PASS:
> >> + metasize = xdp.data - xdp.data_meta;
> >> +
> >> /* recalculate offset to account for any header
> >> - * adjustments. Note other cases do not build an
> >> - * skb and avoid using offset
> >> + * adjustments and minus the metasize to copy the
> >> + * metadata in page_to_skb(). Note other cases do not
> >> + * build an skb and avoid using offset
> >> */
> >> - offset = xdp.data -
> >> - page_address(xdp_page) - vi->hdr_len;
> >> + offset = xdp.data - page_address(xdp_page) -
> >> + vi->hdr_len - metasize;
> >> - /* recalculate len if xdp.data or xdp.data_end were
> >> - * adjusted
> >> + /* recalculate len if xdp.data, xdp.data_end or
> >> + * xdp.data_meta were adjusted
> >> */
> >> - len = xdp.data_end - xdp.data + vi->hdr_len;
> >> + len = xdp.data_end - xdp.data + vi->hdr_len + metasize;
> >> /* We can only create skb based on xdp_page. */
> >> if (unlikely(xdp_page != page)) {
> >> rcu_read_unlock();
> >> put_page(page);
> >> - head_skb = page_to_skb(vi, rq, xdp_page,
> >> - offset, len,
> >> - PAGE_SIZE, false);
> >> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> >> + len, PAGE_SIZE, false,
> >> + metasize);
> >> return head_skb;
> >> }
> >> break;
> >> @@ -921,7 +932,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> goto err_skb;
> >> }
> >> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> >> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> >> + metasize);
> >> curr_skb = head_skb;
> >> if (unlikely(!curr_skb))
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-21 8:36 ` Yuya Kusakabe
2020-02-21 11:01 ` Michael S. Tsirkin
@ 2020-02-23 8:14 ` Michael S. Tsirkin
2020-02-24 4:05 ` Jason Wang
1 sibling, 1 reply; 28+ messages in thread
From: Michael S. Tsirkin @ 2020-02-23 8:14 UTC (permalink / raw)
To: Yuya Kusakabe
Cc: Jason Wang, andriin, ast, bpf, daniel, davem, hawk,
john.fastabend, kafai, kuba, linux-kernel, netdev,
songliubraving, yhs
On Fri, Feb 21, 2020 at 05:36:08PM +0900, Yuya Kusakabe wrote:
> On 2/21/20 1:23 PM, Jason Wang wrote:
> >
> > On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
> >> Implement support for transferring XDP meta data into skb for
> >> virtio_net driver; before calling into the program, xdp.data_meta points
> >> to xdp.data, where on program return with pass verdict, we call
> >> into skb_metadata_set().
> >>
> >> Tested with the script at
> >> https://github.com/higebu/virtio_net-xdp-metadata-test.
> >>
> >> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> >
> >
> > I'm not sure this is correct since virtio-net claims to not support metadata by calling xdp_set_data_meta_invalid()?
>
> virtio_net doesn't support by calling xdp_set_data_meta_invalid() for now.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n686
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n842
>
> And xdp_set_data_meta_invalid() are added by de8f3a83b0a0.
>
> $ git blame ./drivers/net/virtio_net.c | grep xdp_set_data_meta_invalid
> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 686) xdp_set_data_meta_invalid(&xdp);
> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 842) xdp_set_data_meta_invalid(&xdp);
>
> So I added `Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")` to the comment.
>
> >
> >
> >> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> >> ---
> >> v5:
> >> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
> >> - receive_small(): do not copy vnet header if xdp_prog is availavle.
> >> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
> >> - improve comments.
> >> v4:
> >> - improve commit message
> >> v3:
> >> - fix preserve the vnet header in receive_small().
> >> v2:
> >> - keep copy untouched in page_to_skb().
> >> - preserve the vnet header in receive_small().
> >> - fix indentation.
> >> ---
> >> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
> >> 1 file changed, 33 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 2fe7a3188282..4ea0ae60c000 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> struct receive_queue *rq,
> >> struct page *page, unsigned int offset,
> >> unsigned int len, unsigned int truesize,
> >> - bool hdr_valid)
> >> + bool hdr_valid, unsigned int metasize)
> >> {
> >> struct sk_buff *skb;
> >> struct virtio_net_hdr_mrg_rxbuf *hdr;
> >> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> else
> >> hdr_padded_len = sizeof(struct padded_vnet_hdr);
> >> + /* hdr_valid means no XDP, so we can copy the vnet header */
> >> if (hdr_valid)
> >> memcpy(hdr, p, hdr_len);
> >> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >> copy = skb_tailroom(skb);
> >> skb_put_data(skb, p, copy);
> >> + if (metasize) {
> >> + __skb_pull(skb, metasize);
> >> + skb_metadata_set(skb, metasize);
> >> + }
> >> +
> >> len -= copy;
> >> offset += copy;
> >> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
> >> struct virtio_net_hdr_mrg_rxbuf *hdr;
> >> int err;
> >> - /* virtqueue want to use data area in-front of packet */
> >> - if (unlikely(xdpf->metasize > 0))
> >> - return -EOPNOTSUPP;
> >> -
> >> if (unlikely(xdpf->headroom < vi->hdr_len))
> >> return -EOVERFLOW;
> >> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> unsigned int delta = 0;
> >> struct page *xdp_page;
> >> int err;
> >> + unsigned int metasize = 0;
> >> len -= vi->hdr_len;
> >> stats->bytes += len;
> >> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> >> xdp.data = xdp.data_hard_start + xdp_headroom;
> >> - xdp_set_data_meta_invalid(&xdp);
> >> xdp.data_end = xdp.data + len;
> >> + xdp.data_meta = xdp.data;
> >> xdp.rxq = &rq->xdp_rxq;
> >> orig_data = xdp.data;
> >> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> /* Recalculate length in case bpf program changed it */
> >> delta = orig_data - xdp.data;
> >> len = xdp.data_end - xdp.data;
> >> + metasize = xdp.data - xdp.data_meta;
> >> break;
> >> case XDP_TX:
> >> stats->xdp_tx++;
> >> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
> >> }
> >> skb_reserve(skb, headroom - delta);
> >> skb_put(skb, len);
> >> - if (!delta) {
> >> + if (!xdp_prog) {
> >> buf += header_offset;
> >> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
> >> } /* keep zeroed vnet hdr since packet was changed by bpf */
> >
> >
> > I prefer to make this an independent patch and cc stable.
> >
> > Other looks good.
> >
> > Thanks
>
> I see. So I need to revert to delta from xdp_prog?
>
> Thank you.
So maybe send a 2 patch series: 1/2 is this chunk with the appropriate
description. Actually for netdev David prefers that people do not
cc stable directly, just include Fixes tag and mention in the
commit log it's also needed for stable. Patch 2/2 is the rest
handling metadata.
> >
> >> + if (metasize)
> >> + skb_metadata_set(skb, metasize);
> >> +
> >> err:
> >> return skb;
> >> @@ -760,8 +767,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
> >> struct virtnet_rq_stats *stats)
> >> {
> >> struct page *page = buf;
> >> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
> >> - PAGE_SIZE, true);
> >> + struct sk_buff *skb =
> >> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
> >> stats->bytes += len - vi->hdr_len;
> >> if (unlikely(!skb))
> >> @@ -793,6 +800,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> unsigned int truesize;
> >> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> >> int err;
> >> + unsigned int metasize = 0;
> >> head_skb = NULL;
> >> stats->bytes += len - vi->hdr_len;
> >> @@ -839,8 +847,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> data = page_address(xdp_page) + offset;
> >> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
> >> xdp.data = data + vi->hdr_len;
> >> - xdp_set_data_meta_invalid(&xdp);
> >> xdp.data_end = xdp.data + (len - vi->hdr_len);
> >> + xdp.data_meta = xdp.data;
> >> xdp.rxq = &rq->xdp_rxq;
> >> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> @@ -848,24 +856,27 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> switch (act) {
> >> case XDP_PASS:
> >> + metasize = xdp.data - xdp.data_meta;
> >> +
> >> /* recalculate offset to account for any header
> >> - * adjustments. Note other cases do not build an
> >> - * skb and avoid using offset
> >> + * adjustments and minus the metasize to copy the
> >> + * metadata in page_to_skb(). Note other cases do not
> >> + * build an skb and avoid using offset
> >> */
> >> - offset = xdp.data -
> >> - page_address(xdp_page) - vi->hdr_len;
> >> + offset = xdp.data - page_address(xdp_page) -
> >> + vi->hdr_len - metasize;
> >> - /* recalculate len if xdp.data or xdp.data_end were
> >> - * adjusted
> >> + /* recalculate len if xdp.data, xdp.data_end or
> >> + * xdp.data_meta were adjusted
> >> */
> >> - len = xdp.data_end - xdp.data + vi->hdr_len;
> >> + len = xdp.data_end - xdp.data + vi->hdr_len + metasize;
> >> /* We can only create skb based on xdp_page. */
> >> if (unlikely(xdp_page != page)) {
> >> rcu_read_unlock();
> >> put_page(page);
> >> - head_skb = page_to_skb(vi, rq, xdp_page,
> >> - offset, len,
> >> - PAGE_SIZE, false);
> >> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
> >> + len, PAGE_SIZE, false,
> >> + metasize);
> >> return head_skb;
> >> }
> >> break;
> >> @@ -921,7 +932,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >> goto err_skb;
> >> }
> >> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
> >> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
> >> + metasize);
> >> curr_skb = head_skb;
> >> if (unlikely(!curr_skb))
> >
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-23 8:14 ` Michael S. Tsirkin
@ 2020-02-24 4:05 ` Jason Wang
2020-02-25 0:52 ` Yuya Kusakabe
0 siblings, 1 reply; 28+ messages in thread
From: Jason Wang @ 2020-02-24 4:05 UTC (permalink / raw)
To: Michael S. Tsirkin, Yuya Kusakabe
Cc: andriin, ast, bpf, daniel, davem, hawk, john.fastabend, kafai,
kuba, linux-kernel, netdev, songliubraving, yhs
On 2020/2/23 下午4:14, Michael S. Tsirkin wrote:
> On Fri, Feb 21, 2020 at 05:36:08PM +0900, Yuya Kusakabe wrote:
>> On 2/21/20 1:23 PM, Jason Wang wrote:
>>> On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
>>>> Implement support for transferring XDP meta data into skb for
>>>> virtio_net driver; before calling into the program, xdp.data_meta points
>>>> to xdp.data, where on program return with pass verdict, we call
>>>> into skb_metadata_set().
>>>>
>>>> Tested with the script at
>>>> https://github.com/higebu/virtio_net-xdp-metadata-test.
>>>>
>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>> I'm not sure this is correct since virtio-net claims to not support metadata by calling xdp_set_data_meta_invalid()?
>> virtio_net doesn't support by calling xdp_set_data_meta_invalid() for now.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n686
>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n842
>>
>> And xdp_set_data_meta_invalid() are added by de8f3a83b0a0.
>>
>> $ git blame ./drivers/net/virtio_net.c | grep xdp_set_data_meta_invalid
>> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 686) xdp_set_data_meta_invalid(&xdp);
>> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 842) xdp_set_data_meta_invalid(&xdp);
>>
>> So I added `Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")` to the comment.
>>
>>>> Signed-off-by: Yuya Kusakabe<yuya.kusakabe@gmail.com>
>>>> ---
>>>> v5:
>>>> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
>>>> - receive_small(): do not copy vnet header if xdp_prog is availavle.
>>>> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
>>>> - improve comments.
>>>> v4:
>>>> - improve commit message
>>>> v3:
>>>> - fix preserve the vnet header in receive_small().
>>>> v2:
>>>> - keep copy untouched in page_to_skb().
>>>> - preserve the vnet header in receive_small().
>>>> - fix indentation.
>>>> ---
>>>> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
>>>> 1 file changed, 33 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 2fe7a3188282..4ea0ae60c000 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> struct receive_queue *rq,
>>>> struct page *page, unsigned int offset,
>>>> unsigned int len, unsigned int truesize,
>>>> - bool hdr_valid)
>>>> + bool hdr_valid, unsigned int metasize)
>>>> {
>>>> struct sk_buff *skb;
>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> else
>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>> + /* hdr_valid means no XDP, so we can copy the vnet header */
>>>> if (hdr_valid)
>>>> memcpy(hdr, p, hdr_len);
>>>> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>> copy = skb_tailroom(skb);
>>>> skb_put_data(skb, p, copy);
>>>> + if (metasize) {
>>>> + __skb_pull(skb, metasize);
>>>> + skb_metadata_set(skb, metasize);
>>>> + }
>>>> +
>>>> len -= copy;
>>>> offset += copy;
>>>> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>> int err;
>>>> - /* virtqueue want to use data area in-front of packet */
>>>> - if (unlikely(xdpf->metasize > 0))
>>>> - return -EOPNOTSUPP;
>>>> -
>>>> if (unlikely(xdpf->headroom < vi->hdr_len))
>>>> return -EOVERFLOW;
>>>> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> unsigned int delta = 0;
>>>> struct page *xdp_page;
>>>> int err;
>>>> + unsigned int metasize = 0;
>>>> len -= vi->hdr_len;
>>>> stats->bytes += len;
>>>> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>> - xdp_set_data_meta_invalid(&xdp);
>>>> xdp.data_end = xdp.data + len;
>>>> + xdp.data_meta = xdp.data;
>>>> xdp.rxq = &rq->xdp_rxq;
>>>> orig_data = xdp.data;
>>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> /* Recalculate length in case bpf program changed it */
>>>> delta = orig_data - xdp.data;
>>>> len = xdp.data_end - xdp.data;
>>>> + metasize = xdp.data - xdp.data_meta;
>>>> break;
>>>> case XDP_TX:
>>>> stats->xdp_tx++;
>>>> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> }
>>>> skb_reserve(skb, headroom - delta);
>>>> skb_put(skb, len);
>>>> - if (!delta) {
>>>> + if (!xdp_prog) {
>>>> buf += header_offset;
>>>> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>>>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>>> I prefer to make this an independent patch and cc stable.
>>>
>>> Other looks good.
>>>
>>> Thanks
>> I see. So I need to revert to delta from xdp_prog?
>>
>> Thank you.
> So maybe send a 2 patch series: 1/2 is this chunk with the appropriate
> description. Actually for netdev David prefers that people do not
> cc stable directly, just include Fixes tag and mention in the
> commit log it's also needed for stable. Patch 2/2 is the rest
> handling metadata.
+1
Thanks
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH bpf-next v5] virtio_net: add XDP meta data support
2020-02-24 4:05 ` Jason Wang
@ 2020-02-25 0:52 ` Yuya Kusakabe
0 siblings, 0 replies; 28+ messages in thread
From: Yuya Kusakabe @ 2020-02-25 0:52 UTC (permalink / raw)
To: Jason Wang, Michael S. Tsirkin
Cc: andriin, ast, bpf, daniel, davem, hawk, john.fastabend, kafai,
kuba, linux-kernel, netdev, songliubraving, yhs
On 2/24/20 1:05 PM, Jason Wang wrote:
>
> On 2020/2/23 下午4:14, Michael S. Tsirkin wrote:
>> On Fri, Feb 21, 2020 at 05:36:08PM +0900, Yuya Kusakabe wrote:
>>> On 2/21/20 1:23 PM, Jason Wang wrote:
>>>> On 2020/2/20 下午4:55, Yuya Kusakabe wrote:
>>>>> Implement support for transferring XDP meta data into skb for
>>>>> virtio_net driver; before calling into the program, xdp.data_meta points
>>>>> to xdp.data, where on program return with pass verdict, we call
>>>>> into skb_metadata_set().
>>>>>
>>>>> Tested with the script at
>>>>> https://github.com/higebu/virtio_net-xdp-metadata-test.
>>>>>
>>>>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>>>> I'm not sure this is correct since virtio-net claims to not support metadata by calling xdp_set_data_meta_invalid()?
>>> virtio_net doesn't support by calling xdp_set_data_meta_invalid() for now.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n686
>>> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/drivers/net/virtio_net.c?id=e42da4c62abb547d9c9138e0e7fcd1f36057b5e8#n842
>>>
>>> And xdp_set_data_meta_invalid() are added by de8f3a83b0a0.
>>>
>>> $ git blame ./drivers/net/virtio_net.c | grep xdp_set_data_meta_invalid
>>> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 686) xdp_set_data_meta_invalid(&xdp);
>>> de8f3a83b0a0f (Daniel Borkmann 2017-09-25 02:25:51 +0200 842) xdp_set_data_meta_invalid(&xdp);
>>>
>>> So I added `Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")` to the comment.
>>>
>>>>> Signed-off-by: Yuya Kusakabe<yuya.kusakabe@gmail.com>
>>>>> ---
>>>>> v5:
>>>>> - page_to_skb(): copy vnet header if hdr_valid without checking metasize.
>>>>> - receive_small(): do not copy vnet header if xdp_prog is availavle.
>>>>> - __virtnet_xdp_xmit_one(): remove the xdp_set_data_meta_invalid().
>>>>> - improve comments.
>>>>> v4:
>>>>> - improve commit message
>>>>> v3:
>>>>> - fix preserve the vnet header in receive_small().
>>>>> v2:
>>>>> - keep copy untouched in page_to_skb().
>>>>> - preserve the vnet header in receive_small().
>>>>> - fix indentation.
>>>>> ---
>>>>> drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++----------------
>>>>> 1 file changed, 33 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>> index 2fe7a3188282..4ea0ae60c000 100644
>>>>> --- a/drivers/net/virtio_net.c
>>>>> +++ b/drivers/net/virtio_net.c
>>>>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> struct receive_queue *rq,
>>>>> struct page *page, unsigned int offset,
>>>>> unsigned int len, unsigned int truesize,
>>>>> - bool hdr_valid)
>>>>> + bool hdr_valid, unsigned int metasize)
>>>>> {
>>>>> struct sk_buff *skb;
>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>>> @@ -393,6 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> else
>>>>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>>>>> + /* hdr_valid means no XDP, so we can copy the vnet header */
>>>>> if (hdr_valid)
>>>>> memcpy(hdr, p, hdr_len);
>>>>> @@ -405,6 +406,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>>>>> copy = skb_tailroom(skb);
>>>>> skb_put_data(skb, p, copy);
>>>>> + if (metasize) {
>>>>> + __skb_pull(skb, metasize);
>>>>> + skb_metadata_set(skb, metasize);
>>>>> + }
>>>>> +
>>>>> len -= copy;
>>>>> offset += copy;
>>>>> @@ -450,10 +456,6 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>>>>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>>>>> int err;
>>>>> - /* virtqueue want to use data area in-front of packet */
>>>>> - if (unlikely(xdpf->metasize > 0))
>>>>> - return -EOPNOTSUPP;
>>>>> -
>>>>> if (unlikely(xdpf->headroom < vi->hdr_len))
>>>>> return -EOVERFLOW;
>>>>> @@ -644,6 +646,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> unsigned int delta = 0;
>>>>> struct page *xdp_page;
>>>>> int err;
>>>>> + unsigned int metasize = 0;
>>>>> len -= vi->hdr_len;
>>>>> stats->bytes += len;
>>>>> @@ -683,8 +686,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>>> xdp.data = xdp.data_hard_start + xdp_headroom;
>>>>> - xdp_set_data_meta_invalid(&xdp);
>>>>> xdp.data_end = xdp.data + len;
>>>>> + xdp.data_meta = xdp.data;
>>>>> xdp.rxq = &rq->xdp_rxq;
>>>>> orig_data = xdp.data;
>>>>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>>>>> @@ -695,6 +698,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> /* Recalculate length in case bpf program changed it */
>>>>> delta = orig_data - xdp.data;
>>>>> len = xdp.data_end - xdp.data;
>>>>> + metasize = xdp.data - xdp.data_meta;
>>>>> break;
>>>>> case XDP_TX:
>>>>> stats->xdp_tx++;
>>>>> @@ -735,11 +739,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>>> }
>>>>> skb_reserve(skb, headroom - delta);
>>>>> skb_put(skb, len);
>>>>> - if (!delta) {
>>>>> + if (!xdp_prog) {
>>>>> buf += header_offset;
>>>>> memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>>>>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>>>> I prefer to make this an independent patch and cc stable.
>>>>
>>>> Other looks good.
>>>>
>>>> Thanks
>>> I see. So I need to revert to delta from xdp_prog?
>>>
>>> Thank you.
>> So maybe send a 2 patch series: 1/2 is this chunk with the appropriate
>> description. Actually for netdev David prefers that people do not
>> cc stable directly, just include Fixes tag and mention in the
>> commit log it's also needed for stable. Patch 2/2 is the rest
>> handling metadata.
>
>
> +1
>
> Thanks
>
>
Thank you for the detailed explanation. I will make a 2 patch series.
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-02-25 0:52 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-27 8:06 [PATCH bpf-next] virtio_net: add XDP meta data support Yuya Kusakabe
2019-07-01 9:30 ` Jason Wang
2019-07-02 1:00 ` Yuya Kusakabe
2019-07-02 3:15 ` [PATCH bpf-next v2] " Yuya Kusakabe
2019-07-02 3:59 ` Jason Wang
2019-07-02 5:15 ` Yuya Kusakabe
2019-07-02 8:16 ` [PATCH bpf-next v3] " Yuya Kusakabe
2019-07-02 8:33 ` Jason Wang
2019-07-02 14:11 ` Yuya Kusakabe
2019-07-08 22:38 ` Daniel Borkmann
2019-07-09 3:04 ` Jason Wang
2019-07-09 20:03 ` Daniel Borkmann
2019-07-10 2:30 ` Jason Wang
2020-02-03 13:52 ` Yuya Kusakabe
2020-02-04 3:31 ` Jason Wang
2020-02-04 7:16 ` [PATCH bpf-next v4] " Yuya Kusakabe
2020-02-05 4:10 ` Jason Wang
2020-02-05 9:18 ` Yuya Kusakabe
2020-02-06 3:20 ` Jason Wang
2020-02-20 8:55 ` [PATCH bpf-next v5] " Yuya Kusakabe
2020-02-21 4:23 ` Jason Wang
2020-02-21 8:36 ` Yuya Kusakabe
2020-02-21 11:01 ` Michael S. Tsirkin
2020-02-23 8:14 ` Michael S. Tsirkin
2020-02-24 4:05 ` Jason Wang
2020-02-25 0:52 ` Yuya Kusakabe
2020-02-05 5:33 ` [PATCH bpf-next v4] " Michael S. Tsirkin
2020-02-05 9:19 ` Yuya Kusakabe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).