Michael S. Tsirkin
2018-May-02 13:46 UTC
[PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
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 at 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. > > -vladConfused. 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
Vlad Yasevich
2018-May-02 14:00 UTC
[PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
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 at 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
Michael S. Tsirkin
2018-May-02 14:17 UTC
[PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
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 at 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. > -vladSo 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