* [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
@ 2017-06-28 1:54 Jason Wang
2017-06-28 2:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: Jason Wang @ 2017-06-28 1:54 UTC (permalink / raw)
To: mst, virtualization, netdev, linux-kernel
We should allow csumed packet for small buffer, otherwise XDP_PASS
won't work correctly.
Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch is needed for -stable.
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 143d8a9..499fcc9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
void *orig_data;
u32 act;
- if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
+ if (unlikely(hdr->hdr.gso_type))
goto err_xdp;
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 1:54 [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP Jason Wang @ 2017-06-28 2:02 ` Michael S. Tsirkin 2017-06-28 2:14 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2017-06-28 2:02 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: > We should allow csumed packet for small buffer, otherwise XDP_PASS > won't work correctly. > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") > Signed-off-by: Jason Wang <jasowang@redhat.com> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. What do you think? > --- > The patch is needed for -stable. > --- > drivers/net/virtio_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 143d8a9..499fcc9 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > void *orig_data; > u32 act; > > - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags)) > + if (unlikely(hdr->hdr.gso_type)) > goto err_xdp; > > xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len; > -- > 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 2:02 ` Michael S. Tsirkin @ 2017-06-28 2:14 ` Jason Wang 2017-06-28 2:17 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2017-06-28 2:14 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2017年06月28日 10:02, Michael S. Tsirkin wrote: > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: >> We should allow csumed packet for small buffer, otherwise XDP_PASS >> won't work correctly. >> >> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") >> Signed-off-by: Jason Wang <jasowang@redhat.com> > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. > What do you think? I think it's safe. For XDP_PASS, it work like in the past. For XDP_TX, we zero the vnet header. For adjusting header, XDP prog should deal with csum. Thanks > >> --- >> The patch is needed for -stable. >> --- >> drivers/net/virtio_net.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 143d8a9..499fcc9 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >> void *orig_data; >> u32 act; >> >> - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags)) >> + if (unlikely(hdr->hdr.gso_type)) >> goto err_xdp; >> >> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len; >> -- >> 2.7.4 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 2:14 ` Jason Wang @ 2017-06-28 2:17 ` Michael S. Tsirkin 2017-06-28 2:45 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2017-06-28 2:17 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote: > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: > > > We should allow csumed packet for small buffer, otherwise XDP_PASS > > > won't work correctly. > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. > > What do you think? > > I think it's safe. For XDP_PASS, it work like in the past. That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP tools assume it's value. > For XDP_TX, we > zero the vnet header. Again TX offload is disabled, so packets will go out with an invalid checksum. > For adjusting header, XDP prog should deal with csum. > > Thanks That part seems right. > > > > > --- > > > The patch is needed for -stable. > > > --- > > > drivers/net/virtio_net.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 143d8a9..499fcc9 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > > > void *orig_data; > > > u32 act; > > > - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags)) > > > + if (unlikely(hdr->hdr.gso_type)) > > > goto err_xdp; > > > xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len; > > > -- > > > 2.7.4 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 2:17 ` Michael S. Tsirkin @ 2017-06-28 2:45 ` Jason Wang 2017-06-28 3:31 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2017-06-28 2:45 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2017年06月28日 10:17, Michael S. Tsirkin wrote: > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: >> >> On 2017年06月28日 10:02, Michael S. Tsirkin wrote: >>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: >>>> We should allow csumed packet for small buffer, otherwise XDP_PASS >>>> won't work correctly. >>>> >>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. >>> What do you think? >> I think it's safe. For XDP_PASS, it work like in the past. > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP > tools assume it's value. DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment in skbuff.h " * The hardware you're dealing with doesn't calculate the full checksum * (as in CHECKSUM_COMPLETE), but it does parse headers and verify checksums * for specific protocols. For such packets it will set CHECKSUM_UNNECESSARY * if their checksums are okay. skb->csum is still undefined in this case * though. A driver or device must never modify the checksum field in the * packet even if checksum is verified. " The csum is correct I believe? Thanks > >> For XDP_TX, we >> zero the vnet header. > Again TX offload is disabled, so packets will go out with an invalid > checksum. > >> For adjusting header, XDP prog should deal with csum. >> >> Thanks > That part seems right. > >>>> --- >>>> The patch is needed for -stable. >>>> --- >>>> drivers/net/virtio_net.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index 143d8a9..499fcc9 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev, >>>> void *orig_data; >>>> u32 act; >>>> - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags)) >>>> + if (unlikely(hdr->hdr.gso_type)) >>>> goto err_xdp; >>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len; >>>> -- >>>> 2.7.4 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 2:45 ` Jason Wang @ 2017-06-28 3:31 ` Michael S. Tsirkin 2017-06-28 3:40 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2017-06-28 3:31 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: > > > On 2017年06月28日 10:17, Michael S. Tsirkin wrote: > > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: > > > > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote: > > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: > > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS > > > > > won't work correctly. > > > > > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. > > > > What do you think? > > > I think it's safe. For XDP_PASS, it work like in the past. > > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP > > tools assume it's value. > > DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment > in skbuff.h > > > " > * The hardware you're dealing with doesn't calculate the full checksum > * (as in CHECKSUM_COMPLETE), but it does parse headers and verify > checksums > * for specific protocols. For such packets it will set > CHECKSUM_UNNECESSARY > * if their checksums are okay. skb->csum is still undefined in this case > * though. A driver or device must never modify the checksum field in the > * packet even if checksum is verified. > " > > The csum is correct I believe? > > Thanks That's on input. But I think for tun it's output, where that is equivalent to CHECKSUM_NONE > > > > > For XDP_TX, we > > > zero the vnet header. > > Again TX offload is disabled, so packets will go out with an invalid > > checksum. > > > > > For adjusting header, XDP prog should deal with csum. > > > > > > Thanks > > That part seems right. > > > > > > > --- > > > > > The patch is needed for -stable. > > > > > --- > > > > > drivers/net/virtio_net.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 143d8a9..499fcc9 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev, > > > > > void *orig_data; > > > > > u32 act; > > > > > - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags)) > > > > > + if (unlikely(hdr->hdr.gso_type)) > > > > > goto err_xdp; > > > > > xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len; > > > > > -- > > > > > 2.7.4 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 3:31 ` Michael S. Tsirkin @ 2017-06-28 3:40 ` Jason Wang 2017-06-28 4:01 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2017-06-28 3:40 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2017年06月28日 11:31, Michael S. Tsirkin wrote: > On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: >> On 2017年06月28日 10:17, Michael S. Tsirkin wrote: >>> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: >>>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote: >>>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: >>>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS >>>>>> won't work correctly. >>>>>> >>>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") >>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. >>>>> What do you think? >>>> I think it's safe. For XDP_PASS, it work like in the past. >>> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP >>> tools assume it's value. >> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment >> in skbuff.h >> >> >> " >> * The hardware you're dealing with doesn't calculate the full checksum >> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify >> checksums >> * for specific protocols. For such packets it will set >> CHECKSUM_UNNECESSARY >> * if their checksums are okay. skb->csum is still undefined in this case >> * though. A driver or device must never modify the checksum field in the >> * packet even if checksum is verified. >> " >> >> The csum is correct I believe? >> >> Thanks > That's on input. But I think for tun it's output, where that is equivalent > to CHECKSUM_NONE > > Yes, but the comment said: " CKSUM_NONE: * * The skb was already checksummed by the protocol, or a checksum is not * required. * * CHECKSUM_UNNECESSARY: * * This has the same meaning on as CHECKSUM_NONE for checksum offload on * output. * " So still correct I think? Thanks _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 3:40 ` Jason Wang @ 2017-06-28 4:01 ` Michael S. Tsirkin 2017-06-28 12:05 ` Jason Wang 0 siblings, 1 reply; 12+ messages in thread From: Michael S. Tsirkin @ 2017-06-28 4:01 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote: > > > On 2017年06月28日 11:31, Michael S. Tsirkin wrote: > > On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: > > > On 2017年06月28日 10:17, Michael S. Tsirkin wrote: > > > > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: > > > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote: > > > > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: > > > > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS > > > > > > > won't work correctly. > > > > > > > > > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") > > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com> > > > > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. > > > > > > What do you think? > > > > > I think it's safe. For XDP_PASS, it work like in the past. > > > > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP > > > > tools assume it's value. > > > DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment > > > in skbuff.h > > > > > > > > > " > > > * The hardware you're dealing with doesn't calculate the full checksum > > > * (as in CHECKSUM_COMPLETE), but it does parse headers and verify > > > checksums > > > * for specific protocols. For such packets it will set > > > CHECKSUM_UNNECESSARY > > > * if their checksums are okay. skb->csum is still undefined in this case > > > * though. A driver or device must never modify the checksum field in the > > > * packet even if checksum is verified. > > > " > > > > > > The csum is correct I believe? > > > > > > Thanks > > That's on input. But I think for tun it's output, where that is equivalent > > to CHECKSUM_NONE > > > > > > Yes, but the comment said: > > " > CKSUM_NONE: > * > * The skb was already checksummed by the protocol, or a checksum is not > * required. > * > * CHECKSUM_UNNECESSARY: > * > * This has the same meaning on as CHECKSUM_NONE for checksum offload on > * output. > * > " > > So still correct I think? > > Thanks Hmm maybe I mean NEEDS_CHECKSUM actually. I'll need to re-read the spec. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 4:01 ` Michael S. Tsirkin @ 2017-06-28 12:05 ` Jason Wang 2017-07-03 17:03 ` Michael S. Tsirkin [not found] ` <20170703195752-mutt-send-email-mst@kernel.org> 0 siblings, 2 replies; 12+ messages in thread From: Jason Wang @ 2017-06-28 12:05 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2017年06月28日 12:01, Michael S. Tsirkin wrote: > On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote: >> >> On 2017年06月28日 11:31, Michael S. Tsirkin wrote: >>> On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: >>>> On 2017年06月28日 10:17, Michael S. Tsirkin wrote: >>>>> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: >>>>>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote: >>>>>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: >>>>>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS >>>>>>>> won't work correctly. >>>>>>>> >>>>>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") >>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>>>>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. >>>>>>> What do you think? >>>>>> I think it's safe. For XDP_PASS, it work like in the past. >>>>> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP >>>>> tools assume it's value. >>>> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment >>>> in skbuff.h >>>> >>>> >>>> " >>>> * The hardware you're dealing with doesn't calculate the full checksum >>>> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify >>>> checksums >>>> * for specific protocols. For such packets it will set >>>> CHECKSUM_UNNECESSARY >>>> * if their checksums are okay. skb->csum is still undefined in this case >>>> * though. A driver or device must never modify the checksum field in the >>>> * packet even if checksum is verified. >>>> " >>>> >>>> The csum is correct I believe? >>>> >>>> Thanks >>> That's on input. But I think for tun it's output, where that is equivalent >>> to CHECKSUM_NONE >>> >>> >> Yes, but the comment said: >> >> " >> CKSUM_NONE: >> * >> * The skb was already checksummed by the protocol, or a checksum is not >> * required. >> * >> * CHECKSUM_UNNECESSARY: >> * >> * This has the same meaning on as CHECKSUM_NONE for checksum offload on >> * output. >> * >> " >> >> So still correct I think? >> >> Thanks > Hmm maybe I mean NEEDS_CHECKSUM actually. > > I'll need to re-read the spec. > Not sure this is an issue. But if it is, we can probably checksum the packet before passing it to XDP. But it would be a little slow. Thanks _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-06-28 12:05 ` Jason Wang @ 2017-07-03 17:03 ` Michael S. Tsirkin [not found] ` <20170703195752-mutt-send-email-mst@kernel.org> 1 sibling, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2017-07-03 17:03 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Wed, Jun 28, 2017 at 08:05:06PM +0800, Jason Wang wrote: > > > On 2017年06月28日 12:01, Michael S. Tsirkin wrote: > > On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote: > > > > > > On 2017年06月28日 11:31, Michael S. Tsirkin wrote: > > > > On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: > > > > > On 2017年06月28日 10:17, Michael S. Tsirkin wrote: > > > > > > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: > > > > > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote: > > > > > > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: > > > > > > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS > > > > > > > > > won't work correctly. > > > > > > > > > > > > > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") > > > > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com> > > > > > > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. > > > > > > > > What do you think? > > > > > > > I think it's safe. For XDP_PASS, it work like in the past. > > > > > > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP > > > > > > tools assume it's value. > > > > > DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment > > > > > in skbuff.h > > > > > > > > > > > > > > > " > > > > > * The hardware you're dealing with doesn't calculate the full checksum > > > > > * (as in CHECKSUM_COMPLETE), but it does parse headers and verify > > > > > checksums > > > > > * for specific protocols. For such packets it will set > > > > > CHECKSUM_UNNECESSARY > > > > > * if their checksums are okay. skb->csum is still undefined in this case > > > > > * though. A driver or device must never modify the checksum field in the > > > > > * packet even if checksum is verified. > > > > > " > > > > > > > > > > The csum is correct I believe? > > > > > > > > > > Thanks > > > > That's on input. But I think for tun it's output, where that is equivalent > > > > to CHECKSUM_NONE > > > > > > > > > > > Yes, but the comment said: > > > > > > " > > > CKSUM_NONE: > > > * > > > * The skb was already checksummed by the protocol, or a checksum is not > > > * required. > > > * > > > * CHECKSUM_UNNECESSARY: > > > * > > > * This has the same meaning on as CHECKSUM_NONE for checksum offload on > > > * output. > > > * > > > " > > > > > > So still correct I think? > > > > > > Thanks > > Hmm maybe I mean NEEDS_CHECKSUM actually. > > > > I'll need to re-read the spec. > > > > Not sure this is an issue. But if it is, we can probably checksum the packet > before passing it to XDP. But it would be a little slow. > > Thanks Right. I confused DATA_VALID with NEEDS_CHECKSUM. IIUC XDP generally refuses to attach if checksum offload is enabled. Could you pls explain how to reproduce the issue you are seeing? -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20170703195752-mutt-send-email-mst@kernel.org>]
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP [not found] ` <20170703195752-mutt-send-email-mst@kernel.org> @ 2017-07-04 12:20 ` Jason Wang 2017-07-06 0:07 ` Michael S. Tsirkin 0 siblings, 1 reply; 12+ messages in thread From: Jason Wang @ 2017-07-04 12:20 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization On 2017年07月04日 01:03, Michael S. Tsirkin wrote: > On Wed, Jun 28, 2017 at 08:05:06PM +0800, Jason Wang wrote: >> >> On 2017年06月28日 12:01, Michael S. Tsirkin wrote: >>> On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote: >>>> On 2017年06月28日 11:31, Michael S. Tsirkin wrote: >>>>> On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote: >>>>>> On 2017年06月28日 10:17, Michael S. Tsirkin wrote: >>>>>>> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote: >>>>>>>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote: >>>>>>>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote: >>>>>>>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS >>>>>>>>>> won't work correctly. >>>>>>>>>> >>>>>>>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers") >>>>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com> >>>>>>>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set. >>>>>>>>> What do you think? >>>>>>>> I think it's safe. For XDP_PASS, it work like in the past. >>>>>>> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP >>>>>>> tools assume it's value. >>>>>> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment >>>>>> in skbuff.h >>>>>> >>>>>> >>>>>> " >>>>>> * The hardware you're dealing with doesn't calculate the full checksum >>>>>> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify >>>>>> checksums >>>>>> * for specific protocols. For such packets it will set >>>>>> CHECKSUM_UNNECESSARY >>>>>> * if their checksums are okay. skb->csum is still undefined in this case >>>>>> * though. A driver or device must never modify the checksum field in the >>>>>> * packet even if checksum is verified. >>>>>> " >>>>>> >>>>>> The csum is correct I believe? >>>>>> >>>>>> Thanks >>>>> That's on input. But I think for tun it's output, where that is equivalent >>>>> to CHECKSUM_NONE >>>>> >>>>> >>>> Yes, but the comment said: >>>> >>>> " >>>> CKSUM_NONE: >>>> * >>>> * The skb was already checksummed by the protocol, or a checksum is not >>>> * required. >>>> * >>>> * CHECKSUM_UNNECESSARY: >>>> * >>>> * This has the same meaning on as CHECKSUM_NONE for checksum offload on >>>> * output. >>>> * >>>> " >>>> >>>> So still correct I think? >>>> >>>> Thanks >>> Hmm maybe I mean NEEDS_CHECKSUM actually. >>> >>> I'll need to re-read the spec. >>> >> Not sure this is an issue. But if it is, we can probably checksum the packet >> before passing it to XDP. But it would be a little slow. >> >> Thanks > > > Right. I confused DATA_VALID with NEEDS_CHECKSUM. > > IIUC XDP generally refuses to attach if checksum offload > is enabled. Any reason to do this? (Looks like I don't see any code for this) > > Could you pls explain how to reproduce the issue you are seeing? > Using small buffer, all csumed packets will be dropped. Thanks _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP 2017-07-04 12:20 ` Jason Wang @ 2017-07-06 0:07 ` Michael S. Tsirkin 0 siblings, 0 replies; 12+ messages in thread From: Michael S. Tsirkin @ 2017-07-06 0:07 UTC (permalink / raw) To: Jason Wang; +Cc: netdev, linux-kernel, virtualization On Tue, Jul 04, 2017 at 08:20:00PM +0800, Jason Wang wrote: > > IIUC XDP generally refuses to attach if checksum offload > > is enabled. > > Any reason to do this? (Looks like I don't see any code for this) Some of it was covered here https://www.mail-archive.com/netdev@vger.kernel.org/msg162577.html -- MST ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-07-06 0:07 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-28 1:54 [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP Jason Wang
2017-06-28 2:02 ` Michael S. Tsirkin
2017-06-28 2:14 ` Jason Wang
2017-06-28 2:17 ` Michael S. Tsirkin
2017-06-28 2:45 ` Jason Wang
2017-06-28 3:31 ` Michael S. Tsirkin
2017-06-28 3:40 ` Jason Wang
2017-06-28 4:01 ` Michael S. Tsirkin
2017-06-28 12:05 ` Jason Wang
2017-07-03 17:03 ` Michael S. Tsirkin
[not found] ` <20170703195752-mutt-send-email-mst@kernel.org>
2017-07-04 12:20 ` Jason Wang
2017-07-06 0:07 ` Michael S. Tsirkin
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).