| Message ID | 20180502020739.19239-6-vyasevic@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | virtio-net: Add SCTP checksum offload support | expand |
On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: > Since we now have support for software CRC32c offload, turn it on > for macvlan and macvtap devices so that guests can take advantage > of offload SCTP checksums to the host or host hardware. > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- > drivers/net/macvlan.c | 5 +++-- > drivers/net/tap.c | 8 +++++--- > 2 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index 725f4b4..646b730 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > > #define ALWAYS_ON_OFFLOADS \ > (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ > - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) > + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) > > #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) > > @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ > NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ > NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ > - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) > + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ > + NETIF_F_SCTP_CRC) > > #define MACVLAN_STATE_MASK \ > ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) > diff --git a/drivers/net/tap.c b/drivers/net/tap.c > index 9b6cb78..2c8512b 100644 > --- a/drivers/net/tap.c > +++ b/drivers/net/tap.c > @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) > * check, we either support them all or none. > */ > if (skb->ip_summed == CHECKSUM_PARTIAL && > - !(features & NETIF_F_CSUM_MASK) && > - skb_checksum_help(skb)) > + skb_csum_hwoffload_help(skb, features)) > goto drop; > if (ptr_ring_produce(&q->ring, skb)) > goto drop; > @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) > } > } > > + if (arg & TUN_F_SCTP_CSUM) > + feature_mask |= NETIF_F_SCTP_CRC; > + so this still affects TX, shouldn't this affect RX instead? > /* tun/tap driver inverts the usage for TSO offloads, where > * setting the TSO bit means that the userspace wants to > * accept TSO frames and turning it off means that user space > @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, > case TUNSETOFFLOAD: > /* let the user check for future flags */ > if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | > - TUN_F_TSO_ECN | TUN_F_UFO)) > + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) > return -EINVAL; > > rtnl_lock(); > -- > 2.9.5
On 05/01/2018 11:24 PM, Michael S. Tsirkin wrote: > On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: >> Since we now have support for software CRC32c offload, turn it on >> for macvlan and macvtap devices so that guests can take advantage >> of offload SCTP checksums to the host or host hardware. >> >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >> --- >> drivers/net/macvlan.c | 5 +++-- >> drivers/net/tap.c | 8 +++++--- >> 2 files changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c >> index 725f4b4..646b730 100644 >> --- a/drivers/net/macvlan.c >> +++ b/drivers/net/macvlan.c >> @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >> >> #define ALWAYS_ON_OFFLOADS \ >> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ >> - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) >> + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) >> >> #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) >> >> @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ >> NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ >> NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ >> - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) >> + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ >> + NETIF_F_SCTP_CRC) >> >> #define MACVLAN_STATE_MASK \ >> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) >> diff --git a/drivers/net/tap.c b/drivers/net/tap.c >> index 9b6cb78..2c8512b 100644 >> --- a/drivers/net/tap.c >> +++ b/drivers/net/tap.c >> @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) >> * check, we either support them all or none. >> */ >> if (skb->ip_summed == CHECKSUM_PARTIAL && >> - !(features & NETIF_F_CSUM_MASK) && >> - skb_checksum_help(skb)) >> + skb_csum_hwoffload_help(skb, features)) >> goto drop; >> if (ptr_ring_produce(&q->ring, skb)) >> goto drop; >> @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) >> } >> } >> >> + if (arg & TUN_F_SCTP_CSUM) >> + feature_mask |= NETIF_F_SCTP_CRC; >> + > > so this still affects TX, shouldn't this affect RX instead? There is no bit to set on the RX path just like there is no bit to set on the RX patch for TUN_F_CSUM. We only invert TSO offloads, not checksum offloads as the comment below states. For checksum, macvtap has to compute the checksum itself in tap_handle_frame() above. It uses tx feature bits to see if needs do to the checksum. If you think we need another flag to macvtap to control RXCSUM, that would need to be separate and cover standard TCP checksum as well. -vlad > > >> /* tun/tap driver inverts the usage for TSO offloads, where >> * setting the TSO bit means that the userspace wants to >> * accept TSO frames and turning it off means that user space >> @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, >> case TUNSETOFFLOAD: >> /* let the user check for future flags */ >> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | >> - TUN_F_TSO_ECN | TUN_F_UFO)) >> + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) >> return -EINVAL; >> >> rtnl_lock(); >> -- >> 2.9.5
On Wed, May 02, 2018 at 09:27:00AM -0400, Vlad Yasevich wrote: > On 05/01/2018 11:24 PM, Michael S. Tsirkin wrote: > > On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: > >> Since we now have support for software CRC32c offload, turn it on > >> for macvlan and macvtap devices so that guests can take advantage > >> of offload SCTP checksums to the host or host hardware. > >> > >> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > >> --- > >> drivers/net/macvlan.c | 5 +++-- > >> drivers/net/tap.c | 8 +++++--- > >> 2 files changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > >> index 725f4b4..646b730 100644 > >> --- a/drivers/net/macvlan.c > >> +++ b/drivers/net/macvlan.c > >> @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > >> > >> #define ALWAYS_ON_OFFLOADS \ > >> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ > >> - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) > >> + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) > >> > >> #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) > >> > >> @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > >> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ > >> NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ > >> NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ > >> - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) > >> + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ > >> + NETIF_F_SCTP_CRC) > >> > >> #define MACVLAN_STATE_MASK \ > >> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) > >> diff --git a/drivers/net/tap.c b/drivers/net/tap.c > >> index 9b6cb78..2c8512b 100644 > >> --- a/drivers/net/tap.c > >> +++ b/drivers/net/tap.c > >> @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) > >> * check, we either support them all or none. > >> */ > >> if (skb->ip_summed == CHECKSUM_PARTIAL && > >> - !(features & NETIF_F_CSUM_MASK) && > >> - skb_checksum_help(skb)) > >> + skb_csum_hwoffload_help(skb, features)) > >> goto drop; > >> if (ptr_ring_produce(&q->ring, skb)) > >> goto drop; > >> @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) > >> } > >> } > >> > >> + if (arg & TUN_F_SCTP_CSUM) > >> + feature_mask |= NETIF_F_SCTP_CRC; > >> + > > > > so this still affects TX, shouldn't this affect RX instead? > > There is no bit to set on the RX path just like there is no bit to set on the RX patch > for TUN_F_CSUM. > > We only invert TSO offloads, not checksum offloads as the comment below states. > For checksum, macvtap has to compute the checksum itself in tap_handle_frame() above. > It uses tx feature bits to see if needs do to the checksum. > > If you think we need another flag to macvtap to control RXCSUM, that would need to be > separate and cover standard TCP checksum as well. > > -vlad Confused. What is the meaning of TUN_F_SCTP_CSUM? I assume this is a way for userspace to tell tun device: "I can handle packets without SCTP checksum, pls send them my way". Now what is the implication for macvtap? And why are you setting NETIF_F_SCTP_CRC which is a flag that affects packets sent by guest to host? > > > > > >> /* tun/tap driver inverts the usage for TSO offloads, where > >> * setting the TSO bit means that the userspace wants to > >> * accept TSO frames and turning it off means that user space > >> @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, > >> case TUNSETOFFLOAD: > >> /* let the user check for future flags */ > >> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | > >> - TUN_F_TSO_ECN | TUN_F_UFO)) > >> + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) > >> return -EINVAL; > >> > >> rtnl_lock(); > >> -- > >> 2.9.5
On 05/02/2018 09:46 AM, Michael S. Tsirkin wrote: > On Wed, May 02, 2018 at 09:27:00AM -0400, Vlad Yasevich wrote: >> On 05/01/2018 11:24 PM, Michael S. Tsirkin wrote: >>> On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: >>>> Since we now have support for software CRC32c offload, turn it on >>>> for macvlan and macvtap devices so that guests can take advantage >>>> of offload SCTP checksums to the host or host hardware. >>>> >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >>>> --- >>>> drivers/net/macvlan.c | 5 +++-- >>>> drivers/net/tap.c | 8 +++++--- >>>> 2 files changed, 8 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c >>>> index 725f4b4..646b730 100644 >>>> --- a/drivers/net/macvlan.c >>>> +++ b/drivers/net/macvlan.c >>>> @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >>>> >>>> #define ALWAYS_ON_OFFLOADS \ >>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ >>>> - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) >>>> + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) >>>> >>>> #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) >>>> >>>> @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ >>>> NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ >>>> NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ >>>> - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) >>>> + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ >>>> + NETIF_F_SCTP_CRC) >>>> >>>> #define MACVLAN_STATE_MASK \ >>>> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) >>>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c >>>> index 9b6cb78..2c8512b 100644 >>>> --- a/drivers/net/tap.c >>>> +++ b/drivers/net/tap.c >>>> @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) >>>> * check, we either support them all or none. >>>> */ >>>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>> - !(features & NETIF_F_CSUM_MASK) && >>>> - skb_checksum_help(skb)) >>>> + skb_csum_hwoffload_help(skb, features)) >>>> goto drop; >>>> if (ptr_ring_produce(&q->ring, skb)) >>>> goto drop; >>>> @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) >>>> } >>>> } >>>> >>>> + if (arg & TUN_F_SCTP_CSUM) >>>> + feature_mask |= NETIF_F_SCTP_CRC; >>>> + >>> >>> so this still affects TX, shouldn't this affect RX instead? >> >> There is no bit to set on the RX path just like there is no bit to set on the RX patch >> for TUN_F_CSUM. >> >> We only invert TSO offloads, not checksum offloads as the comment below states. >> For checksum, macvtap has to compute the checksum itself in tap_handle_frame() above. >> It uses tx feature bits to see if needs do to the checksum. >> >> If you think we need another flag to macvtap to control RXCSUM, that would need to be >> separate and cover standard TCP checksum as well. >> >> -vlad > > Confused. What is the meaning of TUN_F_SCTP_CSUM? I assume this is > a way for userspace to tell tun device: "I can handle > packets without SCTP checksum, pls send them my way". Yes, just as TUN_F_CSUM means that tun device can handle packets with partial tcp/udp checksum. > > Now what is the implication for macvtap? The implication is exactly the same as for TUN_F_CSUM. If the flag is set on the macvtap device, the TX checksum feature is turned on. > And why are > you setting NETIF_F_SCTP_CRC which is a flag > that affects packets sent by guest to host? Mainly its because we are using just 1 flag to control checksum offloading and we need to be able control both tx and rx paths. What you are suggesting that we either invert what TUN_F_CSUM is doing in macvtap case, or have another flag that lets us control TX and RX paths separately. Either case, that would be separate work. -vlad > > >>> >>> >>>> /* tun/tap driver inverts the usage for TSO offloads, where >>>> * setting the TSO bit means that the userspace wants to >>>> * accept TSO frames and turning it off means that user space >>>> @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, >>>> case TUNSETOFFLOAD: >>>> /* let the user check for future flags */ >>>> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | >>>> - TUN_F_TSO_ECN | TUN_F_UFO)) >>>> + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) >>>> return -EINVAL; >>>> >>>> rtnl_lock(); >>>> -- >>>> 2.9.5
On Wed, May 02, 2018 at 10:00:14AM -0400, Vlad Yasevich wrote: > On 05/02/2018 09:46 AM, Michael S. Tsirkin wrote: > > On Wed, May 02, 2018 at 09:27:00AM -0400, Vlad Yasevich wrote: > >> On 05/01/2018 11:24 PM, Michael S. Tsirkin wrote: > >>> On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: > >>>> Since we now have support for software CRC32c offload, turn it on > >>>> for macvlan and macvtap devices so that guests can take advantage > >>>> of offload SCTP checksums to the host or host hardware. > >>>> > >>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > >>>> --- > >>>> drivers/net/macvlan.c | 5 +++-- > >>>> drivers/net/tap.c | 8 +++++--- > >>>> 2 files changed, 8 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > >>>> index 725f4b4..646b730 100644 > >>>> --- a/drivers/net/macvlan.c > >>>> +++ b/drivers/net/macvlan.c > >>>> @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > >>>> > >>>> #define ALWAYS_ON_OFFLOADS \ > >>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ > >>>> - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) > >>>> + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) > >>>> > >>>> #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) > >>>> > >>>> @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; > >>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ > >>>> NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ > >>>> NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ > >>>> - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) > >>>> + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ > >>>> + NETIF_F_SCTP_CRC) > >>>> > >>>> #define MACVLAN_STATE_MASK \ > >>>> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) > >>>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c > >>>> index 9b6cb78..2c8512b 100644 > >>>> --- a/drivers/net/tap.c > >>>> +++ b/drivers/net/tap.c > >>>> @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) > >>>> * check, we either support them all or none. > >>>> */ > >>>> if (skb->ip_summed == CHECKSUM_PARTIAL && > >>>> - !(features & NETIF_F_CSUM_MASK) && > >>>> - skb_checksum_help(skb)) > >>>> + skb_csum_hwoffload_help(skb, features)) > >>>> goto drop; > >>>> if (ptr_ring_produce(&q->ring, skb)) > >>>> goto drop; > >>>> @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) > >>>> } > >>>> } > >>>> > >>>> + if (arg & TUN_F_SCTP_CSUM) > >>>> + feature_mask |= NETIF_F_SCTP_CRC; > >>>> + > >>> > >>> so this still affects TX, shouldn't this affect RX instead? > >> > >> There is no bit to set on the RX path just like there is no bit to set on the RX patch > >> for TUN_F_CSUM. > >> > >> We only invert TSO offloads, not checksum offloads as the comment below states. > >> For checksum, macvtap has to compute the checksum itself in tap_handle_frame() above. > >> It uses tx feature bits to see if needs do to the checksum. > >> > >> If you think we need another flag to macvtap to control RXCSUM, that would need to be > >> separate and cover standard TCP checksum as well. > >> > >> -vlad > > > > Confused. What is the meaning of TUN_F_SCTP_CSUM? I assume this is > > a way for userspace to tell tun device: "I can handle > > packets without SCTP checksum, pls send them my way". > > Yes, just as TUN_F_CSUM means that tun device can handle packets with > partial tcp/udp checksum. > > > > > Now what is the implication for macvtap? > > The implication is exactly the same as for TUN_F_CSUM. If the > flag is set on the macvtap device, the TX checksum feature is > turned on. I guess I will have to go back and re-read that code - I do not remember what does TUN_F_CSUM does by now. Here is a quick question that might help me: Let's assume userspace does not set TUN_F_SCTP_CSUM and device does not calculate the checksums either. I would expect that with macvtap this then behaves exactly like now with no overhead. > > > And why are > > you setting NETIF_F_SCTP_CRC which is a flag > > that affects packets sent by guest to host? > > Mainly its because we are using just 1 flag to control checksum > offloading and we need to be able control both tx and rx paths. Well that's not really the case I think. What we have is controls for tx offloads for tun. That's TUN_F_CSUM. there are no rx offloads - userspace can send what it wants. These are supposed to translate to rx offloads for macvtap. tx offloads shouldn't be affected at all. Maybe that's not the case - as I said need to go back and check. Will try to find the time in the next couple of days. > What you are suggesting that we either invert what TUN_F_CSUM > is doing in macvtap case, or have another flag that lets us control > TX and RX paths separately. > > Either case, that would be separate work. > -vlad So assuming TUN_F_CSUM affects tx for macvtap do you agree it's a bug? And should we add to it with TUN_F_SCTP_CSUM doing the same? > > > > > >>> > >>> > >>>> /* tun/tap driver inverts the usage for TSO offloads, where > >>>> * setting the TSO bit means that the userspace wants to > >>>> * accept TSO frames and turning it off means that user space > >>>> @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, > >>>> case TUNSETOFFLOAD: > >>>> /* let the user check for future flags */ > >>>> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | > >>>> - TUN_F_TSO_ECN | TUN_F_UFO)) > >>>> + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) > >>>> return -EINVAL; > >>>> > >>>> rtnl_lock(); > >>>> -- > >>>> 2.9.5
On 05/02/2018 10:17 AM, Michael S. Tsirkin wrote: > On Wed, May 02, 2018 at 10:00:14AM -0400, Vlad Yasevich wrote: >> On 05/02/2018 09:46 AM, Michael S. Tsirkin wrote: >>> On Wed, May 02, 2018 at 09:27:00AM -0400, Vlad Yasevich wrote: >>>> On 05/01/2018 11:24 PM, Michael S. Tsirkin wrote: >>>>> On Tue, May 01, 2018 at 10:07:38PM -0400, Vladislav Yasevich wrote: >>>>>> Since we now have support for software CRC32c offload, turn it on >>>>>> for macvlan and macvtap devices so that guests can take advantage >>>>>> of offload SCTP checksums to the host or host hardware. >>>>>> >>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> >>>>>> --- >>>>>> drivers/net/macvlan.c | 5 +++-- >>>>>> drivers/net/tap.c | 8 +++++--- >>>>>> 2 files changed, 8 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c >>>>>> index 725f4b4..646b730 100644 >>>>>> --- a/drivers/net/macvlan.c >>>>>> +++ b/drivers/net/macvlan.c >>>>>> @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >>>>>> >>>>>> #define ALWAYS_ON_OFFLOADS \ >>>>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ >>>>>> - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) >>>>>> + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) >>>>>> >>>>>> #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) >>>>>> >>>>>> @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; >>>>>> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ >>>>>> NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ >>>>>> NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ >>>>>> - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) >>>>>> + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ >>>>>> + NETIF_F_SCTP_CRC) >>>>>> >>>>>> #define MACVLAN_STATE_MASK \ >>>>>> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) >>>>>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c >>>>>> index 9b6cb78..2c8512b 100644 >>>>>> --- a/drivers/net/tap.c >>>>>> +++ b/drivers/net/tap.c >>>>>> @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) >>>>>> * check, we either support them all or none. >>>>>> */ >>>>>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>>>> - !(features & NETIF_F_CSUM_MASK) && >>>>>> - skb_checksum_help(skb)) >>>>>> + skb_csum_hwoffload_help(skb, features)) >>>>>> goto drop; >>>>>> if (ptr_ring_produce(&q->ring, skb)) >>>>>> goto drop; >>>>>> @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) >>>>>> } >>>>>> } >>>>>> >>>>>> + if (arg & TUN_F_SCTP_CSUM) >>>>>> + feature_mask |= NETIF_F_SCTP_CRC; >>>>>> + >>>>> >>>>> so this still affects TX, shouldn't this affect RX instead? >>>> >>>> There is no bit to set on the RX path just like there is no bit to set on the RX patch >>>> for TUN_F_CSUM. >>>> >>>> We only invert TSO offloads, not checksum offloads as the comment below states. >>>> For checksum, macvtap has to compute the checksum itself in tap_handle_frame() above. >>>> It uses tx feature bits to see if needs do to the checksum. >>>> >>>> If you think we need another flag to macvtap to control RXCSUM, that would need to be >>>> separate and cover standard TCP checksum as well. >>>> >>>> -vlad >>> >>> Confused. What is the meaning of TUN_F_SCTP_CSUM? I assume this is >>> a way for userspace to tell tun device: "I can handle >>> packets without SCTP checksum, pls send them my way". >> >> Yes, just as TUN_F_CSUM means that tun device can handle packets with >> partial tcp/udp checksum. >> >>> >>> Now what is the implication for macvtap? >> >> The implication is exactly the same as for TUN_F_CSUM. If the >> flag is set on the macvtap device, the TX checksum feature is >> turned on. > > I guess I will have to go back and re-read that code - I do not remember > what does TUN_F_CSUM does by now. Here is a quick question that might > help me: > > Let's assume userspace does not set TUN_F_SCTP_CSUM and device > does not calculate the checksums either. I would expect that with > macvtap this then behaves exactly like now with no overhead. correct. > >> >>> And why are >>> you setting NETIF_F_SCTP_CRC which is a flag >>> that affects packets sent by guest to host? >> >> Mainly its because we are using just 1 flag to control checksum >> offloading and we need to be able control both tx and rx paths. > > Well that's not really the case I think. What we have is controls for tx > offloads for tun. That's TUN_F_CSUM. > there are no rx offloads - userspace can send what it wants. > > These are supposed to translate to rx offloads for macvtap. > tx offloads shouldn't be affected at all. Ok. This is how it works for TSO, but looks like there was a disconnect on the checksum. In both tun and macvtap case, TUN_F_CSUM controls the tx checksum feature bit. > > Maybe that's not the case - as I said need to go back and check. > Will try to find the time in the next couple of days. > >> What you are suggesting that we either invert what TUN_F_CSUM >> is doing in macvtap case, or have another flag that lets us control >> TX and RX paths separately. >> >> Either case, that would be separate work. >> -vlad > > So assuming TUN_F_CSUM affects tx for macvtap do you > agree it's a bug? And should we add to it with > TUN_F_SCTP_CSUM doing the same? let me think about what it would look like if TUN_F_CSUM controlled the rx bits... -vlad > >>> >>> >>>>> >>>>> >>>>>> /* tun/tap driver inverts the usage for TSO offloads, where >>>>>> * setting the TSO bit means that the userspace wants to >>>>>> * accept TSO frames and turning it off means that user space >>>>>> @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, >>>>>> case TUNSETOFFLOAD: >>>>>> /* let the user check for future flags */ >>>>>> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | >>>>>> - TUN_F_TSO_ECN | TUN_F_UFO)) >>>>>> + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) >>>>>> return -EINVAL; >>>>>> >>>>>> rtnl_lock(); >>>>>> -- >>>>>> 2.9.5
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index 725f4b4..646b730 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -834,7 +834,7 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; #define ALWAYS_ON_OFFLOADS \ (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ - NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) + NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL | NETIF_F_SCTP_CRC) #define ALWAYS_ON_FEATURES (ALWAYS_ON_OFFLOADS | NETIF_F_LLTX) @@ -842,7 +842,8 @@ static struct lock_class_key macvlan_netdev_addr_lock_key; (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \ NETIF_F_GSO | NETIF_F_TSO | NETIF_F_LRO | \ NETIF_F_TSO_ECN | NETIF_F_TSO6 | NETIF_F_GRO | NETIF_F_RXCSUM | \ - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER) + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER | \ + NETIF_F_SCTP_CRC) #define MACVLAN_STATE_MASK \ ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT)) diff --git a/drivers/net/tap.c b/drivers/net/tap.c index 9b6cb78..2c8512b 100644 --- a/drivers/net/tap.c +++ b/drivers/net/tap.c @@ -369,8 +369,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb) * check, we either support them all or none. */ if (skb->ip_summed == CHECKSUM_PARTIAL && - !(features & NETIF_F_CSUM_MASK) && - skb_checksum_help(skb)) + skb_csum_hwoffload_help(skb, features)) goto drop; if (ptr_ring_produce(&q->ring, skb)) goto drop; @@ -945,6 +944,9 @@ static int set_offload(struct tap_queue *q, unsigned long arg) } } + if (arg & TUN_F_SCTP_CSUM) + feature_mask |= NETIF_F_SCTP_CRC; + /* tun/tap driver inverts the usage for TSO offloads, where * setting the TSO bit means that the userspace wants to * accept TSO frames and turning it off means that user space @@ -1077,7 +1079,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd, case TUNSETOFFLOAD: /* let the user check for future flags */ if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 | - TUN_F_TSO_ECN | TUN_F_UFO)) + TUN_F_TSO_ECN | TUN_F_UFO | TUN_F_SCTP_CSUM)) return -EINVAL; rtnl_lock();
Since we now have support for software CRC32c offload, turn it on for macvlan and macvtap devices so that guests can take advantage of offload SCTP checksums to the host or host hardware. Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> --- drivers/net/macvlan.c | 5 +++-- drivers/net/tap.c | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-)