* Re: [PATCH] kvmalloc: always use vmalloc if CONFIG_DEBUG_VM
From: Andrew Morton @ 2018-05-02 0:36 UTC (permalink / raw)
To: Mikulas Patocka
Cc: dm-devel, eric.dumazet, mst, netdev, linux-kernel, Matthew Wilcox,
Michal Hocko, linux-mm, edumazet, virtualization, David Miller,
Vlastimil Babka
In-Reply-To: <alpine.LRH.2.02.1804241229410.23702@file01.intranet.prod.int.rdu2.redhat.com>
On Tue, 24 Apr 2018 12:33:01 -0400 (EDT) Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Tue, 24 Apr 2018, Michal Hocko wrote:
>
> > On Tue 24-04-18 11:30:40, Mikulas Patocka wrote:
> > >
> > >
> > > On Tue, 24 Apr 2018, Michal Hocko wrote:
> > >
> > > > On Mon 23-04-18 20:25:15, Mikulas Patocka wrote:
> > > >
> > > > > Fixing __vmalloc code
> > > > > is easy and it doesn't require cooperation with maintainers.
> > > >
> > > > But it is a hack against the intention of the scope api.
> > >
> > > It is not!
> >
> > This discussion simply doesn't make much sense it seems. The scope API
> > is to document the scope of the reclaim recursion critical section. That
> > certainly is not a utility function like vmalloc.
>
> That 15-line __vmalloc bugfix doesn't prevent you (or any other kernel
> developer) from converting the code to the scope API. You make nonsensical
> excuses.
>
Fun thread!
Winding back to the original problem, I'd state it as
- Caller uses kvmalloc() but passes the address into vmalloc-naive
DMA API and
- Caller uses kvmalloc() but passes the address into kfree()
Yes?
If so, then...
Is there a way in which, in the kvmalloc-called-kmalloc path, we can
tag the slab-allocated memory with a "this memory was allocated with
kvmalloc()" flag? I *think* there's extra per-object storage available
with suitable slab/slub debugging options? Perhaps we could steal one
bit from the redzone, dunno.
If so then we can
a) set that flag in kvmalloc() if the kmalloc() call succeeded
b) check for that flag in the DMA code, WARN if it is set.
c) in kvfree(), clear that flag before calling kfree()
d) in kfree(), check for that flag and go WARN() if set.
So both potential bugs are detected all the time, dependent upon
CONFIG_SLUB_DEBUG (and perhaps other slub config options).
^ permalink raw reply
* [PATCH V2 net-next 0/6] virtio-net: Add SCTP checksum offload support
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
Now that we have SCTP offload capabilities in the kernel, we can add
them to virtio as well. First step is SCTP checksum.
We need a new freature in virtio to negotiate this support since
SCTP is excluded with the stardard checksum and requires a little
bit extra. This series proposes VIRTIO_NET_F_SCTP_CSUM feature bit.
As the "little bit extra", the kernel uses a new bit in the skb
(skb->csum_not_inet) to determine whether to use standard inet checksum
or the SCTP CRC32c checksum. This bit has to be communicated between
the host and the guest. This bit is carried in the vnet header.
Tap and macvtap support is added through an extra feature for the
TUNSETOFFLOAD ioctl. Additionally macvtap will no correctly
do sctp checksumming if the receive doesn't support SCTP offload.
This also turns on sctp offloading for macvlan devices.
As for the perf numbers, I am seeing about a 5% increase in vm-to-vm
and vm-to-hos throughput which is the same as manually disabling
sctp checksumming,since this is exactly what we are emulatting.
Sending outside the host, the increase about 2.5-3%.
Since v1:
- Fixed some old patch bugs that snuck in.
- virtio feature bits moved to high bits
- distinguish between GUEST and HOST features
- Fixed offload control command to control sctp checksum offload
- added ipvlan support
Vladislav Yasevich (6):
virtio: Add support for SCTP checksum offloading
sctp: Handle sctp packets with CHECKSUM_PARTIAL
sctp: Build sctp offload support into the base kernel
tun: Add support for SCTP checksum offload
macvlan/macvtap: Add support for SCTP checksum offload.
ipvlan: Add support for SCTP checksum offload
drivers/net/ipvlan/ipvlan_main.c | 3 ++-
drivers/net/macvlan.c | 5 +++--
drivers/net/tap.c | 8 +++++---
drivers/net/tun.c | 7 ++++++-
drivers/net/virtio_net.c | 14 +++++++++++++-
include/linux/virtio_net.h | 6 ++++++
include/net/sctp/sctp.h | 5 -----
include/uapi/linux/if_tun.h | 1 +
include/uapi/linux/virtio_net.h | 5 +++++
net/Kconfig | 1 +
net/sctp/Kconfig | 1 -
net/sctp/Makefile | 3 ++-
net/sctp/input.c | 11 ++++++++++-
net/sctp/offload.c | 4 +++-
net/sctp/protocol.c | 3 ---
15 files changed, 57 insertions(+), 20 deletions(-)
--
2.9.5
^ permalink raw reply
* [PATCH V2 net-next 1/6] virtio: Add support for SCTP checksum offloading
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
To support SCTP checksum offloading, we need to add a new feature
to virtio_net, so we can negotiate support between the hypervisor
and the guest.
The HOST feature bit signifies offloading support for transmit and
enables device offload features.
The GUEST feature bit signifies offloading support of recieve and
is currently only used by the driver in case of xdp.
That patch also adds an addition virtio_net header flag which
mirrors the skb->csum_not_inet flag. This flags is used to indicate
that is this an SCTP packet that needs its checksum computed by the
lower layer. In this case, the lower layer is the host hypervisor or
possibly HW nic that supporst CRC32c offload.
In the case that GUEST feature bit is flag, it will be possible to
receive a virtio_net header with this bit set, which will set the
corresponding skb bit. SCTP protocol will be updated to correctly
deal with it.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
drivers/net/virtio_net.c | 14 +++++++++++++-
include/linux/virtio_net.h | 6 ++++++
include/uapi/linux/virtio_net.h | 5 +++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..34af280 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2148,6 +2148,8 @@ static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
+ if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_SCTP_CSUM))
+ offloads |= 1ULL << VIRTIO_NET_F_GUEST_SCTP_CSUM;
return virtnet_set_guest_offloads(vi, offloads);
}
@@ -2160,6 +2162,8 @@ static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
return 0;
if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
+ if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_SCTP_CSUM))
+ offloads |= 1ULL << VIRTIO_NET_F_GUEST_SCTP_CSUM;
return virtnet_set_guest_offloads(vi, offloads);
}
@@ -2724,6 +2728,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* Do we support "hardware" checksums? */
if (virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
/* This opens up the world of extra features. */
+
dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_SG;
if (csum)
dev->features |= NETIF_F_HW_CSUM | NETIF_F_SG;
@@ -2746,9 +2751,15 @@ static int virtnet_probe(struct virtio_device *vdev)
dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
/* (!csum && gso) case will be fixed by register_netdev() */
}
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
dev->features |= NETIF_F_RXCSUM;
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_SCTP_CSUM)) {
+ dev->hw_features |= NETIF_F_SCTP_CRC;
+ dev->features |= NETIF_F_SCTP_CRC;
+ }
+
dev->vlan_features = dev->features;
/* MTU range: 68 - 65535 */
@@ -2962,7 +2973,8 @@ static struct virtio_device_id id_table[] = {
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
- VIRTIO_NET_F_SPEED_DUPLEX
+ VIRTIO_NET_F_SPEED_DUPLEX, \
+ VIRTIO_NET_F_HOST_SCTP_CSUM, VIRTIO_NET_F_GUEST_SCTP_CSUM
static unsigned int features[] = {
VIRTNET_FEATURES,
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index f144216..28fffdc 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -39,6 +39,9 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
if (!skb_partial_csum_set(skb, start, off))
return -EINVAL;
+
+ if (hdr->flags & VIRTIO_NET_HDR_F_CSUM_NOT_INET)
+ skb->csum_not_inet = 1;
}
if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
@@ -91,6 +94,9 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
skb_checksum_start_offset(skb));
hdr->csum_offset = __cpu_to_virtio16(little_endian,
skb->csum_offset);
+
+ if (skb->csum_not_inet)
+ hdr->flags |= VIRTIO_NET_HDR_F_CSUM_NOT_INET;
} else if (has_data_valid &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 5de6ed3..9dfca1a 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,10 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_GUEST_SCTP_CSUM 61 /* Guest handles SCTP pks w/ partial
+ * csum */
+#define VIRTIO_NET_F_HOST_SCTP_CSUM 62 /* HOST handles SCTP pkts w/ partial
+ * csum */
#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
#ifndef VIRTIO_NET_NO_LEGACY
@@ -101,6 +105,7 @@ struct virtio_net_config {
struct virtio_net_hdr_v1 {
#define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 /* Use csum_start, csum_offset */
#define VIRTIO_NET_HDR_F_DATA_VALID 2 /* Csum is valid */
+#define VIRTIO_NET_HDR_F_CSUM_NOT_INET 4 /* Checksum is not inet */
__u8 flags;
#define VIRTIO_NET_HDR_GSO_NONE 0 /* Not a GSO frame */
#define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */
--
2.9.5
^ permalink raw reply related
* [PATCH V2 net-next 2/6] sctp: Handle sctp packets with CHECKSUM_PARTIAL
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
With SCTP checksum offload available in virtio, it is now
possible for virtio to receive a sctp packet with CHECKSUM_PARTIAL
set (guest-to-guest traffic). SCTP doesn't really have a
partial checksum like TCP does, because CRC32c can't do partial
additive checksumming. It's all or nothing. So an SCTP packet
with CHECKSUM_PARTIAL will have checksum set to 0. Let SCTP
treat this as a valid checksum if CHECKSUM_PARTIAL is set.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
net/sctp/input.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/sctp/input.c b/net/sctp/input.c
index ba8a6e6..055b8ffa 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -80,8 +80,17 @@ static inline int sctp_rcv_checksum(struct net *net, struct sk_buff *skb)
{
struct sctphdr *sh = sctp_hdr(skb);
__le32 cmp = sh->checksum;
- __le32 val = sctp_compute_cksum(skb, 0);
+ __le32 val = 0;
+ /* In sctp PARTIAL checksum is always 0. This is a case of
+ * a packet received from guest that supports checksum offload.
+ * Assume it's correct as there is really no way to verify,
+ * and we want to avaoid computing it unnecesarily.
+ */
+ if (skb->ip_summed == CHECKSUM_PARTIAL)
+ return 0;
+
+ val = sctp_compute_cksum(skb, 0);
if (val != cmp) {
/* CRC failure, dump it. */
__SCTP_INC_STATS(net, SCTP_MIB_CHECKSUMERRORS);
--
2.9.5
^ permalink raw reply related
* [PATCH V2 net-next 3/6] sctp: Build sctp offload support into the base kernel
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
With the SCTP checksum offload support added to virtio, it is
now possible to get into a situation where SCTP not present
in the kernel, but the feature is negotiated. Handle this just
like we do IPv6 and other modular offloads.
Move the sctp offload out of the sctp module and into the base
kernel.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
include/net/sctp/sctp.h | 5 -----
net/Kconfig | 1 +
net/sctp/Kconfig | 1 -
net/sctp/Makefile | 3 ++-
net/sctp/offload.c | 4 +++-
net/sctp/protocol.c | 3 ---
6 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 72c5b8f..625b45f 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -183,11 +183,6 @@ struct sctp_transport *sctp_epaddr_lookup_transport(
int __net_init sctp_proc_init(struct net *net);
/*
- * sctp/offload.c
- */
-int sctp_offload_init(void);
-
-/*
* sctp/stream_sched.c
*/
void sctp_sched_ops_init(void);
diff --git a/net/Kconfig b/net/Kconfig
index 0428f12..2773f98 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -64,6 +64,7 @@ config INET
bool "TCP/IP networking"
select CRYPTO
select CRYPTO_AES
+ select LIBCRC32C
---help---
These are the protocols used on the Internet and on most local
Ethernets. It is highly recommended to say Y here (this will enlarge
diff --git a/net/sctp/Kconfig b/net/sctp/Kconfig
index c740b18..d07477a 100644
--- a/net/sctp/Kconfig
+++ b/net/sctp/Kconfig
@@ -9,7 +9,6 @@ menuconfig IP_SCTP
select CRYPTO
select CRYPTO_HMAC
select CRYPTO_SHA1
- select LIBCRC32C
---help---
Stream Control Transmission Protocol
diff --git a/net/sctp/Makefile b/net/sctp/Makefile
index e845e45..ee206ca 100644
--- a/net/sctp/Makefile
+++ b/net/sctp/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_IP_SCTP) += sctp.o
obj-$(CONFIG_INET_SCTP_DIAG) += sctp_diag.o
+obj-$(CONFIG_INET) += offload.o
sctp-y := sm_statetable.o sm_statefuns.o sm_sideeffect.o \
protocol.o endpointola.o associola.o \
@@ -12,7 +13,7 @@ sctp-y := sm_statetable.o sm_statefuns.o sm_sideeffect.o \
inqueue.o outqueue.o ulpqueue.o \
tsnmap.o bind_addr.o socket.o primitive.o \
output.o input.o debug.o stream.o auth.o \
- offload.o stream_sched.o stream_sched_prio.o \
+ stream_sched.o stream_sched_prio.o \
stream_sched_rr.o stream_interleave.o
sctp_diag-y := diag.o
diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index 123e9f2..c61cbde 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c
@@ -107,7 +107,7 @@ static const struct skb_checksum_ops crc32c_csum_ops = {
.combine = sctp_csum_combine,
};
-int __init sctp_offload_init(void)
+static int __init sctp_offload_init(void)
{
int ret;
@@ -127,3 +127,5 @@ int __init sctp_offload_init(void)
out:
return ret;
}
+
+fs_initcall(sctp_offload_init);
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index a24cde2..46d2b63 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1479,9 +1479,6 @@ static __init int sctp_init(void)
if (status)
goto err_v6_add_protocol;
- if (sctp_offload_init() < 0)
- pr_crit("%s: Cannot add SCTP protocol offload\n", __func__);
-
out:
return status;
err_v6_add_protocol:
--
2.9.5
^ permalink raw reply related
* [PATCH V2 net-next 4/6] tun: Add support for SCTP checksum offload
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
Adds a new tun offload flag to allow for SCTP checksum offload.
The flag has to be set by the user and defaults to "no offload".
Add SCTP checksum support to the supported tun features.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
drivers/net/tun.c | 7 ++++++-
include/uapi/linux/if_tun.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a1ba262..4f314a6 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -216,7 +216,7 @@ struct tun_struct {
struct net_device *dev;
netdev_features_t set_features;
#define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6)
+ NETIF_F_TSO6|NETIF_F_SCTP_CRC)
int align;
int vnet_hdr_sz;
@@ -2719,6 +2719,11 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
arg &= ~TUN_F_UFO;
}
+ if (arg & TUN_F_SCTP_CSUM) {
+ features |= NETIF_F_SCTP_CRC;
+ arg &= ~TUN_F_SCTP_CSUM;
+ }
+
/* This gives the user a way to test for new features in future by
* trying to set them. */
if (arg)
diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
index ee432cd..061aea8 100644
--- a/include/uapi/linux/if_tun.h
+++ b/include/uapi/linux/if_tun.h
@@ -86,6 +86,7 @@
#define TUN_F_TSO6 0x04 /* I can handle TSO for IPv6 packets */
#define TUN_F_TSO_ECN 0x08 /* I can handle TSO with ECN bits. */
#define TUN_F_UFO 0x10 /* I can handle UFO packets */
+#define TUN_F_SCTP_CSUM 0x20 /* I can handle SCTP checksums offloads */
/* Protocol info prepended to the packets (when IFF_NO_PI is not set) */
#define TUN_PKT_STRIP 0x0001
--
2.9.5
^ permalink raw reply related
* [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
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;
+
/* 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
^ permalink raw reply related
* [PATCH V2 net-next 6/6] ipvlan: Add support for SCTP checksum offload
From: Vladislav Yasevich @ 2018-05-02 2:07 UTC (permalink / raw)
To: netdev
Cc: virtio-dev, marcelo.leitner, nhorman, mst, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-1-vyasevic@redhat.com>
Since ipvlan is a software device, we can turn on SCTP checksum
offload and let the checksum be computed at the lower layer.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
drivers/net/ipvlan/ipvlan_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 450eec2..ddb1590 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -161,7 +161,8 @@ static void ipvlan_port_destroy(struct net_device *dev)
(NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST | \
NETIF_F_GSO | NETIF_F_TSO | NETIF_F_GSO_ROBUST | \
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 IPVLAN_STATE_MASK \
((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
--
2.9.5
^ permalink raw reply related
* Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
From: Jason Wang @ 2018-05-02 2:51 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev; +Cc: wexu
In-Reply-To: <20180425051550.24342-5-tiwei.bie@intel.com>
On 2018年04月25日 13:15, Tiwei Bie wrote:
> This commit introduces the event idx support in packed
> ring. This feature is temporarily disabled, because the
> implementation in this patch may not work as expected,
> and some further discussions on the implementation are
> needed, e.g. do we have to check the wrap counter when
> checking whether a kick is needed?
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/virtio/virtio_ring.c | 53 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 0181e93897be..b1039c2985b9 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> - u16 flags;
> + u16 new, old, off_wrap, flags;
> bool needs_kick;
> u32 snapshot;
>
> @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> * suppressions. */
> virtio_mb(vq->weak_barriers);
>
> + old = vq->next_avail_idx - vq->num_added;
> + new = vq->next_avail_idx;
> + vq->num_added = 0;
> +
> snapshot = *(u32 *)vq->vring_packed.device;
> + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0xffff);
> flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3;
>
> #ifdef DEBUG
> @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> vq->last_add_time_valid = false;
> #endif
>
> - needs_kick = (flags != VRING_EVENT_F_DISABLE);
> + if (flags == VRING_EVENT_F_DESC)
> + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old);
I wonder whether or not the math is correct. Both new and event are in
the unit of descriptor ring size, but old looks not.
Thanks
> + else
> + needs_kick = (flags != VRING_EVENT_F_DISABLE);
> END_USE(vq);
> return needs_kick;
> }
> @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> if (vq->last_used_idx >= vq->vring_packed.num)
> vq->last_used_idx -= vq->vring_packed.num;
>
> + /* If we expect an interrupt for the next entry, tell host
> + * by writing event index and flush out the write before
> + * the read in the next get_buf call. */
> + if (vq->event_flags_shadow == VRING_EVENT_F_DESC)
> + virtio_store_mb(vq->weak_barriers,
> + &vq->vring_packed.driver->off_wrap,
> + cpu_to_virtio16(_vq->vdev, vq->last_used_idx |
> + (vq->wrap_counter << 15)));
> +
> #ifdef DEBUG
> vq->last_add_time_valid = false;
> #endif
> @@ -1143,10 +1160,17 @@ static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
>
> /* We optimistically turn back on interrupts, then check if there was
> * more to do. */
> + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> + * either clear the flags bit or point the event index at the next
> + * entry. Always update the event index to keep code simple. */
> +
> + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> + vq->last_used_idx | (vq->wrap_counter << 15));
>
> if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> virtio_wmb(vq->weak_barriers);
> - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> + VRING_EVENT_F_ENABLE;
> vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> vq->event_flags_shadow);
> }
> @@ -1172,15 +1196,34 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
> static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + u16 bufs, used_idx, wrap_counter;
>
> START_USE(vq);
>
> /* We optimistically turn back on interrupts, then check if there was
> * more to do. */
> + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> + * either clear the flags bit or point the event index at the next
> + * entry. Always update the event index to keep code simple. */
> +
> + /* TODO: tune this threshold */
> + bufs = (u16)(vq->next_avail_idx - vq->last_used_idx) * 3 / 4;
> +
> + used_idx = vq->last_used_idx + bufs;
> + wrap_counter = vq->wrap_counter;
> +
> + if (used_idx >= vq->vring_packed.num) {
> + used_idx -= vq->vring_packed.num;
> + wrap_counter ^= 1;
> + }
> +
> + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> + used_idx | (wrap_counter << 15));
>
> if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> virtio_wmb(vq->weak_barriers);
> - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> + VRING_EVENT_F_ENABLE;
> vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> vq->event_flags_shadow);
> }
> @@ -1822,8 +1865,10 @@ void vring_transport_features(struct virtio_device *vdev)
> switch (i) {
> case VIRTIO_RING_F_INDIRECT_DESC:
> break;
> +#if 0
> case VIRTIO_RING_F_EVENT_IDX:
> break;
> +#endif
> case VIRTIO_F_VERSION_1:
> break;
> case VIRTIO_F_IOMMU_PLATFORM:
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH V2 net-next 1/6] virtio: Add support for SCTP checksum offloading
From: Michael S. Tsirkin @ 2018-05-02 3:16 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-2-vyasevic@redhat.com>
On Tue, May 01, 2018 at 10:07:34PM -0400, Vladislav Yasevich wrote:
> To support SCTP checksum offloading, we need to add a new feature
> to virtio_net, so we can negotiate support between the hypervisor
> and the guest.
> The HOST feature bit signifies offloading support for transmit and
> enables device offload features.
> The GUEST feature bit signifies offloading support of recieve and
> is currently only used by the driver in case of xdp.
>
> That patch also adds an addition virtio_net header flag which
> mirrors the skb->csum_not_inet flag. This flags is used to indicate
> that is this an SCTP packet that needs its checksum computed by the
> lower layer. In this case, the lower layer is the host hypervisor or
> possibly HW nic that supporst CRC32c offload.
>
> In the case that GUEST feature bit is flag, it will be possible to
> receive a virtio_net header with this bit set, which will set the
> corresponding skb bit. SCTP protocol will be updated to correctly
> deal with it.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
> drivers/net/virtio_net.c | 14 +++++++++++++-
> include/linux/virtio_net.h | 6 ++++++
> include/uapi/linux/virtio_net.h | 5 +++++
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7b187ec..34af280 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2148,6 +2148,8 @@ static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
>
> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
> offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_SCTP_CSUM))
> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_SCTP_CSUM;
>
> return virtnet_set_guest_offloads(vi, offloads);
> }
> @@ -2160,6 +2162,8 @@ static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
> return 0;
> if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
> offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_SCTP_CSUM))
> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_SCTP_CSUM;
>
> return virtnet_set_guest_offloads(vi, offloads);
> }
> @@ -2724,6 +2728,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* Do we support "hardware" checksums? */
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
> /* This opens up the world of extra features. */
> +
> dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_SG;
> if (csum)
> dev->features |= NETIF_F_HW_CSUM | NETIF_F_SG;
> @@ -2746,9 +2751,15 @@ static int virtnet_probe(struct virtio_device *vdev)
> dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
> /* (!csum && gso) case will be fixed by register_netdev() */
> }
> +
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> dev->features |= NETIF_F_RXCSUM;
>
> + if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_SCTP_CSUM)) {
> + dev->hw_features |= NETIF_F_SCTP_CRC;
> + dev->features |= NETIF_F_SCTP_CRC;
> + }
> +
> dev->vlan_features = dev->features;
>
> /* MTU range: 68 - 65535 */
> @@ -2962,7 +2973,8 @@ static struct virtio_device_id id_table[] = {
> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> - VIRTIO_NET_F_SPEED_DUPLEX
> + VIRTIO_NET_F_SPEED_DUPLEX, \
> + VIRTIO_NET_F_HOST_SCTP_CSUM, VIRTIO_NET_F_GUEST_SCTP_CSUM
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index f144216..28fffdc 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -39,6 +39,9 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>
> if (!skb_partial_csum_set(skb, start, off))
> return -EINVAL;
> +
> + if (hdr->flags & VIRTIO_NET_HDR_F_CSUM_NOT_INET)
> + skb->csum_not_inet = 1;
> }
>
> if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> @@ -91,6 +94,9 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
> skb_checksum_start_offset(skb));
> hdr->csum_offset = __cpu_to_virtio16(little_endian,
> skb->csum_offset);
> +
> + if (skb->csum_not_inet)
> + hdr->flags |= VIRTIO_NET_HDR_F_CSUM_NOT_INET;
> } else if (has_data_valid &&
> skb->ip_summed == CHECKSUM_UNNECESSARY) {
> hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 5de6ed3..9dfca1a 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -57,6 +57,10 @@
> * Steering */
> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>
> +#define VIRTIO_NET_F_GUEST_SCTP_CSUM 61 /* Guest handles SCTP pks w/ partial
> + * csum */
> +#define VIRTIO_NET_F_HOST_SCTP_CSUM 62 /* HOST handles SCTP pkts w/ partial
> + * csum */
> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>
> #ifndef VIRTIO_NET_NO_LEGACY
> @@ -101,6 +105,7 @@ struct virtio_net_config {
> struct virtio_net_hdr_v1 {
> #define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 /* Use csum_start, csum_offset */
> #define VIRTIO_NET_HDR_F_DATA_VALID 2 /* Csum is valid */
> +#define VIRTIO_NET_HDR_F_CSUM_NOT_INET 4 /* Checksum is not inet */
Both comment and name are not very informative.
How about just saying CRC32c ?
> __u8 flags;
> #define VIRTIO_NET_HDR_GSO_NONE 0 /* Not a GSO frame */
> #define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */
> --
> 2.9.5
^ permalink raw reply
* Re: [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Michael S. Tsirkin @ 2018-05-02 3:24 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp
In-Reply-To: <20180502020739.19239-6-vyasevic@redhat.com>
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
^ permalink raw reply
* Re: [RFC v3 0/5] virtio: support packed ring
From: Jason Wang @ 2018-05-02 3:49 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev; +Cc: wexu
In-Reply-To: <20180425051550.24342-1-tiwei.bie@intel.com>
On 2018年04月25日 13:15, Tiwei Bie wrote:
> Hello everyone,
>
> This RFC implements packed ring support in virtio driver.
>
> Some simple functional tests have been done with Jason's
> packed ring implementation in vhost:
>
> https://lkml.org/lkml/2018/4/23/12
>
> Both of ping and netperf worked as expected (with EVENT_IDX
> disabled). But there are below known issues:
>
> 1. Reloading the guest driver will break the Tx/Rx;
It looks like the reason is we don't sync wrap counter information
between host and qemu through VHOST_SET/GET_VRING_BASE. And both vhost
and qemu need to do this through encoding warp counters to higher bits
of vhost_vring_state.num,
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
From: Tiwei Bie @ 2018-05-02 7:28 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <34781052-df9f-e505-cd3f-08e460b34dcc@redhat.com>
On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote:
> On 2018年04月25日 13:15, Tiwei Bie wrote:
> > This commit introduces the event idx support in packed
> > ring. This feature is temporarily disabled, because the
> > implementation in this patch may not work as expected,
> > and some further discussions on the implementation are
> > needed, e.g. do we have to check the wrap counter when
> > checking whether a kick is needed?
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > drivers/virtio/virtio_ring.c | 53 ++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 49 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 0181e93897be..b1039c2985b9 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > - u16 flags;
> > + u16 new, old, off_wrap, flags;
> > bool needs_kick;
> > u32 snapshot;
> > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > * suppressions. */
> > virtio_mb(vq->weak_barriers);
> > + old = vq->next_avail_idx - vq->num_added;
> > + new = vq->next_avail_idx;
> > + vq->num_added = 0;
> > +
> > snapshot = *(u32 *)vq->vring_packed.device;
> > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0xffff);
> > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3;
> > #ifdef DEBUG
> > @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > vq->last_add_time_valid = false;
> > #endif
> > - needs_kick = (flags != VRING_EVENT_F_DISABLE);
> > + if (flags == VRING_EVENT_F_DESC)
> > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old);
>
> I wonder whether or not the math is correct. Both new and event are in the
> unit of descriptor ring size, but old looks not.
What vring_need_event() cares is the distance between
`new` and `old`, i.e. vq->num_added. So I think there
is nothing wrong with `old`. But the calculation of the
distance between `new` and `event_idx` isn't right when
`new` wraps. How do you think about the below code:
wrap_counter = off_wrap >> 15;
event_idx = off_wrap & ~(1<<15);
if (wrap_counter != vq->wrap_counter)
event_idx -= vq->vring_packed.num;
needs_kick = vring_need_event(event_idx, new, old);
Best regards,
Tiwei Bie
>
> Thanks
>
> > + else
> > + needs_kick = (flags != VRING_EVENT_F_DISABLE);
> > END_USE(vq);
> > return needs_kick;
> > }
> > @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > if (vq->last_used_idx >= vq->vring_packed.num)
> > vq->last_used_idx -= vq->vring_packed.num;
> > + /* If we expect an interrupt for the next entry, tell host
> > + * by writing event index and flush out the write before
> > + * the read in the next get_buf call. */
> > + if (vq->event_flags_shadow == VRING_EVENT_F_DESC)
> > + virtio_store_mb(vq->weak_barriers,
> > + &vq->vring_packed.driver->off_wrap,
> > + cpu_to_virtio16(_vq->vdev, vq->last_used_idx |
> > + (vq->wrap_counter << 15)));
> > +
> > #ifdef DEBUG
> > vq->last_add_time_valid = false;
> > #endif
> > @@ -1143,10 +1160,17 @@ static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
> > /* We optimistically turn back on interrupts, then check if there was
> > * more to do. */
> > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > + * either clear the flags bit or point the event index at the next
> > + * entry. Always update the event index to keep code simple. */
> > +
> > + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> > + vq->last_used_idx | (vq->wrap_counter << 15));
> > if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> > virtio_wmb(vq->weak_barriers);
> > - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> > + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> > + VRING_EVENT_F_ENABLE;
> > vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> > vq->event_flags_shadow);
> > }
> > @@ -1172,15 +1196,34 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
> > static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> > {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > + u16 bufs, used_idx, wrap_counter;
> > START_USE(vq);
> > /* We optimistically turn back on interrupts, then check if there was
> > * more to do. */
> > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > + * either clear the flags bit or point the event index at the next
> > + * entry. Always update the event index to keep code simple. */
> > +
> > + /* TODO: tune this threshold */
> > + bufs = (u16)(vq->next_avail_idx - vq->last_used_idx) * 3 / 4;
> > +
> > + used_idx = vq->last_used_idx + bufs;
> > + wrap_counter = vq->wrap_counter;
> > +
> > + if (used_idx >= vq->vring_packed.num) {
> > + used_idx -= vq->vring_packed.num;
> > + wrap_counter ^= 1;
> > + }
> > +
> > + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> > + used_idx | (wrap_counter << 15));
> > if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> > virtio_wmb(vq->weak_barriers);
> > - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> > + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> > + VRING_EVENT_F_ENABLE;
> > vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> > vq->event_flags_shadow);
> > }
> > @@ -1822,8 +1865,10 @@ void vring_transport_features(struct virtio_device *vdev)
> > switch (i) {
> > case VIRTIO_RING_F_INDIRECT_DESC:
> > break;
> > +#if 0
> > case VIRTIO_RING_F_EVENT_IDX:
> > break;
> > +#endif
> > case VIRTIO_F_VERSION_1:
> > break;
> > case VIRTIO_F_IOMMU_PLATFORM:
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v9 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Jiri Pirko @ 2018-05-02 7:50 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, mst, kubakici, netdev,
virtualization, loseweigh, aaron.f.brown, davem
In-Reply-To: <9c6f055c-c665-0601-3973-bce74d72544b@intel.com>
Wed, May 02, 2018 at 02:20:26AM CEST, sridhar.samudrala@intel.com wrote:
>On 4/30/2018 12:20 AM, Jiri Pirko wrote:
>>
>> > > Now I try to change mac of the failover master:
>> > > [root@test1 ~]# ip link set ens3 addr 52:54:00:b2:a7:f3
>> > > RTNETLINK answers: Operation not supported
>> > >
>> > > That I did expect to work. I would expect this would change the mac of
>> > > the master and both standby and primary slaves.
>> > If a VF is untrusted, a VM will not able to change its MAC and moreover
>> Note that at this point, I have no VF. So I'm not sure why you mention
>> that.
>>
>>
>> > in this mode we are assuming that the hypervisor has assigned the MAC and
>> > guest is not expected to change the MAC.
>> Wait, for ordinary old-fashioned virtio_net, as a VM user, I can change
>> mac and all works fine. How is this different? Change mac on "failover
>> instance" should work and should propagate the mac down to its slaves.
>>
>>
>> > For the initial implementation, i would propose not allowing the guest to
>> > change the MAC of failover or standby dev.
>> I see no reason for such restriction.
>>
>
>It is true that a VM user can change mac address of a normal virtio-net interface,
>however when it is in STANDBY mode i think we should not allow this change specifically
>because we are creating a failover instance based on a MAC that is assigned by the
>hypervisor.
>
>Moreover, in a cloud environment i would think that PF/hypervisor assigns a MAC to
>the VF and it cannot be changed by the guest.
So that is easy. You allow the change of the mac and in the "failover"
mac change implementation you propagate the change down to slaves. If
one slave does not support the change, you bail out. And since VF does
not allow it as you say, once it will be enslaved, the mac change could
not be done. Seems like a correct behavior to me and is in-sync with how
bond/team behaves.
>
>So for the initial implementation, do you see any issues with having this restriction
>in STANDBY mode.
>
>
^ permalink raw reply
* Re: [RFC V3 PATCH 1/8] vhost: move get_rx_bufs to vhost.c
From: Tiwei Bie @ 2018-05-02 8:05 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, mst, netdev, linux-kernel, virtualization, wexu
In-Reply-To: <1524461700-5469-2-git-send-email-jasowang@redhat.com>
On Mon, Apr 23, 2018 at 01:34:53PM +0800, Jason Wang wrote:
> Move get_rx_bufs() to vhost.c and rename it to
> vhost_get_rx_bufs(). This helps to hide vring internal layout from
A small typo. Based on the code change in this patch, it
seems that this function is renamed to vhost_get_bufs().
Thanks
> specific device implementation. Packed ring implementation will
> benefit from this.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 83 ++-------------------------------------------------
> drivers/vhost/vhost.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/vhost.h | 7 +++++
> 3 files changed, 88 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 986058a..762aa81 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -664,83 +664,6 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
> return len;
> }
>
> -/* This is a multi-buffer version of vhost_get_desc, that works if
> - * vq has read descriptors only.
> - * @vq - the relevant virtqueue
> - * @datalen - data length we'll be reading
> - * @iovcount - returned count of io vectors we fill
> - * @log - vhost log
> - * @log_num - log offset
> - * @quota - headcount quota, 1 for big buffer
> - * returns number of buffer heads allocated, negative on error
> - */
> -static int get_rx_bufs(struct vhost_virtqueue *vq,
> - struct vring_used_elem *heads,
> - int datalen,
> - unsigned *iovcount,
> - struct vhost_log *log,
> - unsigned *log_num,
> - unsigned int quota)
> -{
> - unsigned int out, in;
> - int seg = 0;
> - int headcount = 0;
> - unsigned d;
> - int r, nlogs = 0;
> - /* len is always initialized before use since we are always called with
> - * datalen > 0.
> - */
> - u32 uninitialized_var(len);
> -
> - while (datalen > 0 && headcount < quota) {
> - if (unlikely(seg >= UIO_MAXIOV)) {
> - r = -ENOBUFS;
> - goto err;
> - }
> - r = vhost_get_vq_desc(vq, vq->iov + seg,
> - ARRAY_SIZE(vq->iov) - seg, &out,
> - &in, log, log_num);
> - if (unlikely(r < 0))
> - goto err;
> -
> - d = r;
> - if (d == vq->num) {
> - r = 0;
> - goto err;
> - }
> - if (unlikely(out || in <= 0)) {
> - vq_err(vq, "unexpected descriptor format for RX: "
> - "out %d, in %d\n", out, in);
> - r = -EINVAL;
> - goto err;
> - }
> - if (unlikely(log)) {
> - nlogs += *log_num;
> - log += *log_num;
> - }
> - heads[headcount].id = cpu_to_vhost32(vq, d);
> - len = iov_length(vq->iov + seg, in);
> - heads[headcount].len = cpu_to_vhost32(vq, len);
> - datalen -= len;
> - ++headcount;
> - seg += in;
> - }
> - heads[headcount - 1].len = cpu_to_vhost32(vq, len + datalen);
> - *iovcount = seg;
> - if (unlikely(log))
> - *log_num = nlogs;
> -
> - /* Detect overrun */
> - if (unlikely(datalen > 0)) {
> - r = UIO_MAXIOV + 1;
> - goto err;
> - }
> - return headcount;
> -err:
> - vhost_discard_vq_desc(vq, headcount);
> - return r;
> -}
> -
> /* Expects to be always run from workqueue - which acts as
> * read-size critical section for our kind of RCU. */
> static void handle_rx(struct vhost_net *net)
> @@ -790,9 +713,9 @@ static void handle_rx(struct vhost_net *net)
> while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk))) {
> sock_len += sock_hlen;
> vhost_len = sock_len + vhost_hlen;
> - headcount = get_rx_bufs(vq, vq->heads + nheads, vhost_len,
> - &in, vq_log, &log,
> - likely(mergeable) ? UIO_MAXIOV : 1);
> + headcount = vhost_get_bufs(vq, vq->heads + nheads, vhost_len,
> + &in, vq_log, &log,
> + likely(mergeable) ? UIO_MAXIOV : 1);
> /* On error, stop handling until the next kick. */
> if (unlikely(headcount < 0))
> goto out;
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f3bd8e9..6b455f6 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2097,6 +2097,84 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> }
> EXPORT_SYMBOL_GPL(vhost_get_vq_desc);
>
> +/* This is a multi-buffer version of vhost_get_desc, that works if
> + * vq has read descriptors only.
> + * @vq - the relevant virtqueue
> + * @datalen - data length we'll be reading
> + * @iovcount - returned count of io vectors we fill
> + * @log - vhost log
> + * @log_num - log offset
> + * @quota - headcount quota, 1 for big buffer
> + * returns number of buffer heads allocated, negative on error
> + */
> +int vhost_get_bufs(struct vhost_virtqueue *vq,
> + struct vring_used_elem *heads,
> + int datalen,
> + unsigned *iovcount,
> + struct vhost_log *log,
> + unsigned *log_num,
> + unsigned int quota)
> +{
> + unsigned int out, in;
> + int seg = 0;
> + int headcount = 0;
> + unsigned d;
> + int r, nlogs = 0;
> + /* len is always initialized before use since we are always called with
> + * datalen > 0.
> + */
> + u32 uninitialized_var(len);
> +
> + while (datalen > 0 && headcount < quota) {
> + if (unlikely(seg >= UIO_MAXIOV)) {
> + r = -ENOBUFS;
> + goto err;
> + }
> + r = vhost_get_vq_desc(vq, vq->iov + seg,
> + ARRAY_SIZE(vq->iov) - seg, &out,
> + &in, log, log_num);
> + if (unlikely(r < 0))
> + goto err;
> +
> + d = r;
> + if (d == vq->num) {
> + r = 0;
> + goto err;
> + }
> + if (unlikely(out || in <= 0)) {
> + vq_err(vq, "unexpected descriptor format for RX: "
> + "out %d, in %d\n", out, in);
> + r = -EINVAL;
> + goto err;
> + }
> + if (unlikely(log)) {
> + nlogs += *log_num;
> + log += *log_num;
> + }
> + heads[headcount].id = cpu_to_vhost32(vq, d);
> + len = iov_length(vq->iov + seg, in);
> + heads[headcount].len = cpu_to_vhost32(vq, len);
> + datalen -= len;
> + ++headcount;
> + seg += in;
> + }
> + heads[headcount - 1].len = cpu_to_vhost32(vq, len + datalen);
> + *iovcount = seg;
> + if (unlikely(log))
> + *log_num = nlogs;
> +
> + /* Detect overrun */
> + if (unlikely(datalen > 0)) {
> + r = UIO_MAXIOV + 1;
> + goto err;
> + }
> + return headcount;
> +err:
> + vhost_discard_vq_desc(vq, headcount);
> + return r;
> +}
> +EXPORT_SYMBOL_GPL(vhost_get_bufs);
> +
> /* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
> void vhost_discard_vq_desc(struct vhost_virtqueue *vq, int n)
> {
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 6c844b9..52edd242 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -185,6 +185,13 @@ int vhost_get_vq_desc(struct vhost_virtqueue *,
> struct iovec iov[], unsigned int iov_count,
> unsigned int *out_num, unsigned int *in_num,
> struct vhost_log *log, unsigned int *log_num);
> +int vhost_get_bufs(struct vhost_virtqueue *vq,
> + struct vring_used_elem *heads,
> + int datalen,
> + unsigned *iovcount,
> + struct vhost_log *log,
> + unsigned *log_num,
> + unsigned int quota);
> void vhost_discard_vq_desc(struct vhost_virtqueue *, int n);
>
> int vhost_vq_init_access(struct vhost_virtqueue *);
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH] drm/qxl: Remove unecessary dma_fence_ops
From: Daniel Vetter @ 2018-05-02 8:23 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, virtualization,
Eric Anholt, Dave Airlie
The trivial enable_signaling implementation matches the default code.
v2: Fix up commit message to match patch better (Eric).
Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux-foundation.org
---
drivers/gpu/drm/qxl/qxl_release.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 7cb214577275..e37f0097f744 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -50,12 +50,6 @@ static const char *qxl_get_timeline_name(struct dma_fence *fence)
return "release";
}
-static bool qxl_nop_signaling(struct dma_fence *fence)
-{
- /* fences are always automatically signaled, so just pretend we did this.. */
- return true;
-}
-
static long qxl_fence_wait(struct dma_fence *fence, bool intr,
signed long timeout)
{
@@ -119,7 +113,6 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr,
static const struct dma_fence_ops qxl_fence_ops = {
.get_driver_name = qxl_get_driver_name,
.get_timeline_name = qxl_get_timeline_name,
- .enable_signaling = qxl_nop_signaling,
.wait = qxl_fence_wait,
};
--
2.17.0
^ permalink raw reply related
* [PATCH] drm/qxl: Remove unecessary dma_fence_ops
From: Daniel Vetter @ 2018-05-02 8:23 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, virtualization,
Eric Anholt, Dave Airlie
In-Reply-To: <20180427061724.28497-13-daniel.vetter@ffwll.ch>
The trivial enable_signaling implementation matches the default code.
v2: Fix up commit message to match patch better (Eric).
Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: virtualization@lists.linux-foundation.org
---
drivers/gpu/drm/qxl/qxl_release.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 7cb214577275..e37f0097f744 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -50,12 +50,6 @@ static const char *qxl_get_timeline_name(struct dma_fence *fence)
return "release";
}
-static bool qxl_nop_signaling(struct dma_fence *fence)
-{
- /* fences are always automatically signaled, so just pretend we did this.. */
- return true;
-}
-
static long qxl_fence_wait(struct dma_fence *fence, bool intr,
signed long timeout)
{
@@ -119,7 +113,6 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr,
static const struct dma_fence_ops qxl_fence_ops = {
.get_driver_name = qxl_get_driver_name,
.get_timeline_name = qxl_get_timeline_name,
- .enable_signaling = qxl_nop_signaling,
.wait = qxl_fence_wait,
};
--
2.17.0
^ permalink raw reply related
* Re: [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Vlad Yasevich @ 2018-05-02 13:27 UTC (permalink / raw)
To: Michael S. Tsirkin, Vladislav Yasevich
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp
In-Reply-To: <20180502062400-mutt-send-email-mst@kernel.org>
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
^ permalink raw reply
* Re: kvmalloc: always use vmalloc if CONFIG_DEBUG_VM
From: Mike Snitzer @ 2018-05-02 13:33 UTC (permalink / raw)
To: Andrew Morton
Cc: eric.dumazet, mst, netdev, linux-kernel, Matthew Wilcox,
Michal Hocko, linux-mm, dm-devel, Mikulas Patocka, Babka,
virtualization, David Miller, edumazet
In-Reply-To: <20180501173626.4593a87d0d64f6cc9d219d20@linux-foundation.org>
On Tue, May 01 2018 at 8:36pm -0400,
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 24 Apr 2018 12:33:01 -0400 (EDT) Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> >
> >
> > On Tue, 24 Apr 2018, Michal Hocko wrote:
> >
> > > On Tue 24-04-18 11:30:40, Mikulas Patocka wrote:
> > > >
> > > >
> > > > On Tue, 24 Apr 2018, Michal Hocko wrote:
> > > >
> > > > > On Mon 23-04-18 20:25:15, Mikulas Patocka wrote:
> > > > >
> > > > > > Fixing __vmalloc code
> > > > > > is easy and it doesn't require cooperation with maintainers.
> > > > >
> > > > > But it is a hack against the intention of the scope api.
> > > >
> > > > It is not!
> > >
> > > This discussion simply doesn't make much sense it seems. The scope API
> > > is to document the scope of the reclaim recursion critical section. That
> > > certainly is not a utility function like vmalloc.
> >
> > That 15-line __vmalloc bugfix doesn't prevent you (or any other kernel
> > developer) from converting the code to the scope API. You make nonsensical
> > excuses.
> >
>
> Fun thread!
>
> Winding back to the original problem, I'd state it as
>
> - Caller uses kvmalloc() but passes the address into vmalloc-naive
> DMA API and
>
> - Caller uses kvmalloc() but passes the address into kfree()
>
> Yes?
I think so.
> If so, then...
>
> Is there a way in which, in the kvmalloc-called-kmalloc path, we can
> tag the slab-allocated memory with a "this memory was allocated with
> kvmalloc()" flag? I *think* there's extra per-object storage available
> with suitable slab/slub debugging options? Perhaps we could steal one
> bit from the redzone, dunno.
>
> If so then we can
>
> a) set that flag in kvmalloc() if the kmalloc() call succeeded
>
> b) check for that flag in the DMA code, WARN if it is set.
>
> c) in kvfree(), clear that flag before calling kfree()
>
> d) in kfree(), check for that flag and go WARN() if set.
>
> So both potential bugs are detected all the time, dependent upon
> CONFIG_SLUB_DEBUG (and perhaps other slub config options).
Thanks Andrew, definitely the most sane proposal I've seen to resolve
this.
^ permalink raw reply
* Re: [PATCH] vhost: make msg padding explicit
From: Michael S. Tsirkin @ 2018-05-02 13:36 UTC (permalink / raw)
To: Kevin Easton; +Cc: kvm, netdev, linux-kernel, virtualization, David Miller
In-Reply-To: <20180502062809.GA14485@la.guarana.org>
On Wed, May 02, 2018 at 02:28:09AM -0400, Kevin Easton wrote:
> On Tue, May 01, 2018 at 02:05:51PM -0400, David Miller wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > Date: Tue, 1 May 2018 20:19:19 +0300
> >
> > > On Tue, May 01, 2018 at 11:28:22AM -0400, David Miller wrote:
> > >> From: "Michael S. Tsirkin" <mst@redhat.com>
> > >> Date: Fri, 27 Apr 2018 19:02:05 +0300
> > >>
> > >> > There's a 32 bit hole just after type. It's best to
> > >> > give it a name, this way compiler is forced to initialize
> > >> > it with rest of the structure.
> > >> >
> > >> > Reported-by: Kevin Easton <kevin@guarana.org>
> > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >>
> > >> Michael, will you be sending this directly to Linus or would you like
> > >> me to apply it to net or net-next?
> > >>
> > >> Thanks.
> > >
> > > I'd prefer you to apply it for net and cc stable if possible.
> >
> > Ok, applied, and added to my -stable submission queue.
>
> Hold on, this patch changes the layout for i386 (where there is
> no padding at all). And it's part of UAPI.
>
> - Kevin
>
> >
Ouch. True - and in particular the 32 bit ABI on 64 bit kernels doesn't
work at all. Hmm. It's relatively new and maybe there aren't any 32 bit
users yet. Thoughts?
--
MST
^ permalink raw reply
* Re: [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Michael S. Tsirkin @ 2018-05-02 13:46 UTC (permalink / raw)
To: Vlad Yasevich
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp, Vladislav Yasevich
In-Reply-To: <35def050-3803-42f4-5ee3-c10f98186c6d@redhat.com>
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
^ permalink raw reply
* Re: [RFC v3 4/5] virtio_ring: add event idx support in packed ring
From: Michael S. Tsirkin @ 2018-05-02 13:51 UTC (permalink / raw)
To: Tiwei Bie; +Cc: netdev, linux-kernel, virtualization, wexu
In-Reply-To: <20180502072819.mf5l3dypk6dwx2s7@debian>
On Wed, May 02, 2018 at 03:28:19PM +0800, Tiwei Bie wrote:
> On Wed, May 02, 2018 at 10:51:06AM +0800, Jason Wang wrote:
> > On 2018年04月25日 13:15, Tiwei Bie wrote:
> > > This commit introduces the event idx support in packed
> > > ring. This feature is temporarily disabled, because the
> > > implementation in this patch may not work as expected,
> > > and some further discussions on the implementation are
> > > needed, e.g. do we have to check the wrap counter when
> > > checking whether a kick is needed?
> > >
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > ---
> > > drivers/virtio/virtio_ring.c | 53 ++++++++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 49 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 0181e93897be..b1039c2985b9 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -986,7 +986,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > > {
> > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > - u16 flags;
> > > + u16 new, old, off_wrap, flags;
> > > bool needs_kick;
> > > u32 snapshot;
> > > @@ -995,7 +995,12 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > > * suppressions. */
> > > virtio_mb(vq->weak_barriers);
> > > + old = vq->next_avail_idx - vq->num_added;
> > > + new = vq->next_avail_idx;
> > > + vq->num_added = 0;
> > > +
> > > snapshot = *(u32 *)vq->vring_packed.device;
> > > + off_wrap = virtio16_to_cpu(_vq->vdev, snapshot & 0xffff);
> > > flags = cpu_to_virtio16(_vq->vdev, snapshot >> 16) & 0x3;
> > > #ifdef DEBUG
> > > @@ -1006,7 +1011,10 @@ static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
> > > vq->last_add_time_valid = false;
> > > #endif
> > > - needs_kick = (flags != VRING_EVENT_F_DISABLE);
> > > + if (flags == VRING_EVENT_F_DESC)
> > > + needs_kick = vring_need_event(off_wrap & ~(1<<15), new, old);
> >
> > I wonder whether or not the math is correct. Both new and event are in the
> > unit of descriptor ring size, but old looks not.
>
> What vring_need_event() cares is the distance between
> `new` and `old`, i.e. vq->num_added. So I think there
> is nothing wrong with `old`. But the calculation of the
> distance between `new` and `event_idx` isn't right when
> `new` wraps. How do you think about the below code:
>
> wrap_counter = off_wrap >> 15;
> event_idx = off_wrap & ~(1<<15);
> if (wrap_counter != vq->wrap_counter)
> event_idx -= vq->vring_packed.num;
>
> needs_kick = vring_need_event(event_idx, new, old);
I suspect this hack won't work for non power of 2 ring.
> Best regards,
> Tiwei Bie
>
>
> >
> > Thanks
> >
> > > + else
> > > + needs_kick = (flags != VRING_EVENT_F_DISABLE);
> > > END_USE(vq);
> > > return needs_kick;
> > > }
> > > @@ -1116,6 +1124,15 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > if (vq->last_used_idx >= vq->vring_packed.num)
> > > vq->last_used_idx -= vq->vring_packed.num;
> > > + /* If we expect an interrupt for the next entry, tell host
> > > + * by writing event index and flush out the write before
> > > + * the read in the next get_buf call. */
> > > + if (vq->event_flags_shadow == VRING_EVENT_F_DESC)
> > > + virtio_store_mb(vq->weak_barriers,
> > > + &vq->vring_packed.driver->off_wrap,
> > > + cpu_to_virtio16(_vq->vdev, vq->last_used_idx |
> > > + (vq->wrap_counter << 15)));
> > > +
> > > #ifdef DEBUG
> > > vq->last_add_time_valid = false;
> > > #endif
> > > @@ -1143,10 +1160,17 @@ static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
> > > /* We optimistically turn back on interrupts, then check if there was
> > > * more to do. */
> > > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > > + * either clear the flags bit or point the event index at the next
> > > + * entry. Always update the event index to keep code simple. */
> > > +
> > > + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> > > + vq->last_used_idx | (vq->wrap_counter << 15));
> > > if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> > > virtio_wmb(vq->weak_barriers);
> > > - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> > > + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> > > + VRING_EVENT_F_ENABLE;
> > > vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> > > vq->event_flags_shadow);
> > > }
> > > @@ -1172,15 +1196,34 @@ static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
> > > static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> > > {
> > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > + u16 bufs, used_idx, wrap_counter;
> > > START_USE(vq);
> > > /* We optimistically turn back on interrupts, then check if there was
> > > * more to do. */
> > > + /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
> > > + * either clear the flags bit or point the event index at the next
> > > + * entry. Always update the event index to keep code simple. */
> > > +
> > > + /* TODO: tune this threshold */
> > > + bufs = (u16)(vq->next_avail_idx - vq->last_used_idx) * 3 / 4;
> > > +
> > > + used_idx = vq->last_used_idx + bufs;
> > > + wrap_counter = vq->wrap_counter;
> > > +
> > > + if (used_idx >= vq->vring_packed.num) {
> > > + used_idx -= vq->vring_packed.num;
> > > + wrap_counter ^= 1;
> > > + }
> > > +
> > > + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> > > + used_idx | (wrap_counter << 15));
> > > if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> > > virtio_wmb(vq->weak_barriers);
> > > - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> > > + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> > > + VRING_EVENT_F_ENABLE;
> > > vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> > > vq->event_flags_shadow);
> > > }
> > > @@ -1822,8 +1865,10 @@ void vring_transport_features(struct virtio_device *vdev)
> > > switch (i) {
> > > case VIRTIO_RING_F_INDIRECT_DESC:
> > > break;
> > > +#if 0
> > > case VIRTIO_RING_F_EVENT_IDX:
> > > break;
> > > +#endif
> > > case VIRTIO_F_VERSION_1:
> > > break;
> > > case VIRTIO_F_IOMMU_PLATFORM:
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Vlad Yasevich @ 2018-05-02 14:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp, Vladislav Yasevich
In-Reply-To: <20180502164317-mutt-send-email-mst@kernel.org>
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
^ permalink raw reply
* Re: [PATCH] vhost: make msg padding explicit
From: David Miller @ 2018-05-02 14:04 UTC (permalink / raw)
To: mst; +Cc: kevin, kvm, netdev, linux-kernel, virtualization
In-Reply-To: <20180502162932-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 2 May 2018 16:36:37 +0300
> Ouch. True - and in particular the 32 bit ABI on 64 bit kernels doesn't
> work at all. Hmm. It's relatively new and maybe there aren't any 32 bit
> users yet. Thoughts?
If it's been in a released kernel version, we really aren't at liberty
to play "maybe nobody uses this" UAPI changing games.
Please send me a revert.
^ permalink raw reply
* Re: [PATCH V2 net-next 5/6] macvlan/macvtap: Add support for SCTP checksum offload.
From: Michael S. Tsirkin @ 2018-05-02 14:17 UTC (permalink / raw)
To: Vlad Yasevich
Cc: virtio-dev, marcelo.leitner, nhorman, netdev, virtualization,
linux-sctp, Vladislav Yasevich
In-Reply-To: <cd94e936-f24c-de0d-7254-069054e33268@redhat.com>
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox