* [PATCH 1/1] Update my email address
From: Cornelia Huck @ 2017-07-04 9:30 UTC (permalink / raw)
To: linux-s390, kvm, virtualization; +Cc: schwidefsky, Cornelia Huck
In-Reply-To: <20170704093038.13943-1-cohuck@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
MAINTAINERS | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 767e9d202adf..84155d593bba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7285,7 +7285,7 @@ F: arch/powerpc/kvm/
KERNEL VIRTUAL MACHINE for s390 (KVM/s390)
M: Christian Borntraeger <borntraeger@de.ibm.com>
-M: Cornelia Huck <cornelia.huck@de.ibm.com>
+M: Cornelia Huck <cohuck@redhat.com>
L: linux-s390@vger.kernel.org
W: http://www.ibm.com/developerworks/linux/linux390/
T: git git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git
@@ -11060,7 +11060,7 @@ S: Supported
F: drivers/iommu/s390-iommu.c
S390 VFIO-CCW DRIVER
-M: Cornelia Huck <cornelia.huck@de.ibm.com>
+M: Cornelia Huck <cohuck@redhat.com>
M: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
L: linux-s390@vger.kernel.org
L: kvm@vger.kernel.org
@@ -13569,7 +13569,7 @@ F: include/uapi/linux/virtio_*.h
F: drivers/crypto/virtio/
VIRTIO DRIVERS FOR S390
-M: Cornelia Huck <cornelia.huck@de.ibm.com>
+M: Cornelia Huck <cohuck@redhat.com>
M: Halil Pasic <pasic@linux.vnet.ibm.com>
L: linux-s390@vger.kernel.org
L: virtualization@lists.linux-foundation.org
--
2.13.0
^ permalink raw reply related
* [PATCH 0/1] Change email address
From: Cornelia Huck @ 2017-07-04 9:30 UTC (permalink / raw)
To: linux-s390, kvm, virtualization; +Cc: schwidefsky, Cornelia Huck
New employer, new address. Make sure people use the right one.
Christian, Martin, it's probably quickest if one of you takes this.
Cornelia Huck (1):
Update my email address
MAINTAINERS | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--
2.13.0
^ permalink raw reply
* Re: [PATCH v2] virtio-blk: add DISCARD support to virtio-blk driver
From: Paolo Bonzini @ 2017-07-04 9:24 UTC (permalink / raw)
To: Changpeng Liu, virtualization; +Cc: hch, mst
In-Reply-To: <1499244279-3484-1-git-send-email-changpeng.liu@intel.com>
On 05/07/2017 10:44, Changpeng Liu wrote:
> Currently virtio-blk driver does not provide discard feature flag, so the
> filesystems which built on top of the block device will not send discard
> command. This is okay for HDD backend, but it will impact the performance
> for SSD backend.
>
> Add a feature flag VIRTIO_BLK_F_DISCARD and command VIRTIO_BLK_T_DISCARD
> to extend exist virtio-blk protocol, define 16 bytes discard descriptor
> for each discard segment, the discard segment defination aligns with
> SCSI or NVM Express protocols, virtio-blk driver will support multi-range
> discard request as well.
>
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Please include a patch for the specification. Since we are at it, I
would like to have three operations defined using the same descriptor:
- discard (SCSI UNMAP)
- write zeroes (SCSI WRITE SAME without UNMAP flag)
- write zeroes and possibly discard (SCSI WRITE SAME with UNMAP flag)
The last two can use the same command VIRTIO_BLK_T_WRITE_ZEROES, using
the reserved field as a flags field.
Paolo
> ---
> drivers/block/virtio_blk.c | 76 +++++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_blk.h | 19 +++++++++++
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 0297ad7..8f0c614 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -172,10 +172,52 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr,
> return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
> }
>
> +static inline int virtblk_setup_discard(struct request *req)
> +{
> + unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
> + u32 block_size = queue_logical_block_size(req->q);
> + struct virtio_blk_discard *range;
> + struct bio *bio;
> +
> + if (block_size < 512 || !block_size)
> + return -1;
> +
> + range = kmalloc_array(segments, sizeof(*range), GFP_ATOMIC);
> + if (!range)
> + return -1;
> +
> + __rq_for_each_bio(bio, req) {
> + u64 slba = (bio->bi_iter.bi_sector << 9) / block_size;
> + u32 nlb = bio->bi_iter.bi_size / block_size;
> +
> + range[n].reserved = cpu_to_le32(0);
> + range[n].nlba = cpu_to_le32(nlb);
> + range[n].slba = cpu_to_le64(slba);
> + n++;
> + }
> +
> + if (WARN_ON_ONCE(n != segments)) {
> + kfree(range);
> + return -1;
> + }
> +
> + req->special_vec.bv_page = virt_to_page(range);
> + req->special_vec.bv_offset = offset_in_page(range);
> + req->special_vec.bv_len = sizeof(*range) * segments;
> + req->rq_flags |= RQF_SPECIAL_PAYLOAD;
> +
> + return 0;
> +}
> +
> static inline void virtblk_request_done(struct request *req)
> {
> struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
>
> + if (req->rq_flags & RQF_SPECIAL_PAYLOAD) {
> + kfree(page_address(req->special_vec.bv_page) +
> + req->special_vec.bv_offset);
> + }
> +
> switch (req_op(req)) {
> case REQ_OP_SCSI_IN:
> case REQ_OP_SCSI_OUT:
> @@ -237,6 +279,9 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
> case REQ_OP_FLUSH:
> type = VIRTIO_BLK_T_FLUSH;
> break;
> + case REQ_OP_DISCARD:
> + type = VIRTIO_BLK_T_DISCARD;
> + break;
> case REQ_OP_SCSI_IN:
> case REQ_OP_SCSI_OUT:
> type = VIRTIO_BLK_T_SCSI_CMD;
> @@ -256,9 +301,15 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
>
> blk_mq_start_request(req);
>
> + if (type == VIRTIO_BLK_T_DISCARD) {
> + err = virtblk_setup_discard(req);
> + if (err)
> + return BLK_STS_IOERR;
> + }
> +
> num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
> if (num) {
> - if (rq_data_dir(req) == WRITE)
> + if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD)
> vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_OUT);
> else
> vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_IN);
> @@ -767,6 +818,25 @@ static int virtblk_probe(struct virtio_device *vdev)
> if (!err && opt_io_size)
> blk_queue_io_opt(q, blk_size * opt_io_size);
>
> + if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
> + q->limits.discard_alignment = blk_size;
> + q->limits.discard_granularity = blk_size;
> +
> + virtio_cread(vdev, struct virtio_blk_config, max_discard_seg, &v);
> + if (v)
> + blk_queue_max_discard_sectors(q, v);
> + else
> + blk_queue_max_discard_sectors(q, -1U);
> +
> + virtio_cread(vdev, struct virtio_blk_config, max_discard_num, &v);
> + if (v)
> + blk_queue_max_discard_segments(q, v);
> + else
> + blk_queue_max_discard_segments(q, 256);
> +
> + queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
> + }
> +
> virtio_device_ready(vdev);
>
> device_add_disk(&vdev->dev, vblk->disk);
> @@ -874,14 +944,14 @@ static int virtblk_restore(struct virtio_device *vdev)
> VIRTIO_BLK_F_SCSI,
> #endif
> VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
> - VIRTIO_BLK_F_MQ,
> + VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
> }
> ;
> static unsigned int features[] = {
> VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
> VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
> VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
> - VIRTIO_BLK_F_MQ,
> + VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
> };
>
> static struct virtio_driver virtio_blk = {
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index 9ebe4d9..3354cc3 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -38,6 +38,7 @@
> #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/
> #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
> #define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
> +#define VIRTIO_BLK_F_DISCARD 13 /* DISCARD command is supported */
>
> /* Legacy feature bits */
> #ifndef VIRTIO_BLK_NO_LEGACY
> @@ -86,6 +87,10 @@ struct virtio_blk_config {
>
> /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
> __u16 num_queues;
> + /* The maximum segment size (if VIRTIO_BLK_F_DISCARD) */
> + __u32 max_discard_seg;
> + /* The maximum number of segments (if VIRTIO_BLK_F_DISCARD) */
> + __u32 max_discard_num;
> } __attribute__((packed));
>
> /*
> @@ -114,6 +119,9 @@ struct virtio_blk_config {
> /* Get device ID command */
> #define VIRTIO_BLK_T_GET_ID 8
>
> +/* Discard command */
> +#define VIRTIO_BLK_T_DISCARD 16
> +
> #ifndef VIRTIO_BLK_NO_LEGACY
> /* Barrier before this op. */
> #define VIRTIO_BLK_T_BARRIER 0x80000000
> @@ -133,6 +141,17 @@ struct virtio_blk_outhdr {
> __virtio64 sector;
> };
>
> +/*
> + * Array of discard ranges for each request.
> + */
> +struct virtio_blk_discard {
> + /* start discard lba */
> + __virtio64 slba;
> + /* number of discard sectors */
> + __virtio32 nlba;
> + __virtio32 reserved;
> +};
> +
> #ifndef VIRTIO_BLK_NO_LEGACY
> struct virtio_scsi_inhdr {
> __virtio32 errors;
>
^ permalink raw reply
* [PATCH v2] virtio-blk: add DISCARD support to virtio-blk driver
From: Changpeng Liu @ 2017-07-04 8:26 UTC (permalink / raw)
To: changpeng.liu, virtualization; +Cc: mst, hch, pbonzini
Currently virtio-blk driver does not provide discard feature flag, so the
filesystems which built on top of the block device will not send discard
command. This is okay for HDD backend, but it will impact the performance
for SSD backend.
Add a feature flag VIRTIO_BLK_F_DISCARD and command VIRTIO_BLK_T_DISCARD
to extend exist virtio-blk protocol, define 16 bytes discard descriptor
for each discard segment, the discard segment defination aligns with
SCSI or NVM Express protocols, virtio-blk driver will support multi-range
discard request as well.
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
drivers/block/virtio_blk.c | 76 +++++++++++++++++++++++++++++++++++++++--
include/uapi/linux/virtio_blk.h | 19 +++++++++++
2 files changed, 92 insertions(+), 3 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0297ad7..8f0c614 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -172,10 +172,52 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr,
return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
}
+static inline int virtblk_setup_discard(struct request *req)
+{
+ unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
+ u32 block_size = queue_logical_block_size(req->q);
+ struct virtio_blk_discard *range;
+ struct bio *bio;
+
+ if (block_size < 512 || !block_size)
+ return -1;
+
+ range = kmalloc_array(segments, sizeof(*range), GFP_ATOMIC);
+ if (!range)
+ return -1;
+
+ __rq_for_each_bio(bio, req) {
+ u64 slba = (bio->bi_iter.bi_sector << 9) / block_size;
+ u32 nlb = bio->bi_iter.bi_size / block_size;
+
+ range[n].reserved = cpu_to_le32(0);
+ range[n].nlba = cpu_to_le32(nlb);
+ range[n].slba = cpu_to_le64(slba);
+ n++;
+ }
+
+ if (WARN_ON_ONCE(n != segments)) {
+ kfree(range);
+ return -1;
+ }
+
+ req->special_vec.bv_page = virt_to_page(range);
+ req->special_vec.bv_offset = offset_in_page(range);
+ req->special_vec.bv_len = sizeof(*range) * segments;
+ req->rq_flags |= RQF_SPECIAL_PAYLOAD;
+
+ return 0;
+}
+
static inline void virtblk_request_done(struct request *req)
{
struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
+ if (req->rq_flags & RQF_SPECIAL_PAYLOAD) {
+ kfree(page_address(req->special_vec.bv_page) +
+ req->special_vec.bv_offset);
+ }
+
switch (req_op(req)) {
case REQ_OP_SCSI_IN:
case REQ_OP_SCSI_OUT:
@@ -237,6 +279,9 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
case REQ_OP_FLUSH:
type = VIRTIO_BLK_T_FLUSH;
break;
+ case REQ_OP_DISCARD:
+ type = VIRTIO_BLK_T_DISCARD;
+ break;
case REQ_OP_SCSI_IN:
case REQ_OP_SCSI_OUT:
type = VIRTIO_BLK_T_SCSI_CMD;
@@ -256,9 +301,15 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
blk_mq_start_request(req);
+ if (type == VIRTIO_BLK_T_DISCARD) {
+ err = virtblk_setup_discard(req);
+ if (err)
+ return BLK_STS_IOERR;
+ }
+
num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
if (num) {
- if (rq_data_dir(req) == WRITE)
+ if (rq_data_dir(req) == WRITE || type == VIRTIO_BLK_T_DISCARD)
vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_OUT);
else
vbr->out_hdr.type |= cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_IN);
@@ -767,6 +818,25 @@ static int virtblk_probe(struct virtio_device *vdev)
if (!err && opt_io_size)
blk_queue_io_opt(q, blk_size * opt_io_size);
+ if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
+ q->limits.discard_alignment = blk_size;
+ q->limits.discard_granularity = blk_size;
+
+ virtio_cread(vdev, struct virtio_blk_config, max_discard_seg, &v);
+ if (v)
+ blk_queue_max_discard_sectors(q, v);
+ else
+ blk_queue_max_discard_sectors(q, -1U);
+
+ virtio_cread(vdev, struct virtio_blk_config, max_discard_num, &v);
+ if (v)
+ blk_queue_max_discard_segments(q, v);
+ else
+ blk_queue_max_discard_segments(q, 256);
+
+ queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
+ }
+
virtio_device_ready(vdev);
device_add_disk(&vdev->dev, vblk->disk);
@@ -874,14 +944,14 @@ static int virtblk_restore(struct virtio_device *vdev)
VIRTIO_BLK_F_SCSI,
#endif
VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
- VIRTIO_BLK_F_MQ,
+ VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
}
;
static unsigned int features[] = {
VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
- VIRTIO_BLK_F_MQ,
+ VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD,
};
static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 9ebe4d9..3354cc3 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -38,6 +38,7 @@
#define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/
#define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
#define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
+#define VIRTIO_BLK_F_DISCARD 13 /* DISCARD command is supported */
/* Legacy feature bits */
#ifndef VIRTIO_BLK_NO_LEGACY
@@ -86,6 +87,10 @@ struct virtio_blk_config {
/* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
__u16 num_queues;
+ /* The maximum segment size (if VIRTIO_BLK_F_DISCARD) */
+ __u32 max_discard_seg;
+ /* The maximum number of segments (if VIRTIO_BLK_F_DISCARD) */
+ __u32 max_discard_num;
} __attribute__((packed));
/*
@@ -114,6 +119,9 @@ struct virtio_blk_config {
/* Get device ID command */
#define VIRTIO_BLK_T_GET_ID 8
+/* Discard command */
+#define VIRTIO_BLK_T_DISCARD 16
+
#ifndef VIRTIO_BLK_NO_LEGACY
/* Barrier before this op. */
#define VIRTIO_BLK_T_BARRIER 0x80000000
@@ -133,6 +141,17 @@ struct virtio_blk_outhdr {
__virtio64 sector;
};
+/*
+ * Array of discard ranges for each request.
+ */
+struct virtio_blk_discard {
+ /* start discard lba */
+ __virtio64 slba;
+ /* number of discard sectors */
+ __virtio32 nlba;
+ __virtio32 reserved;
+};
+
#ifndef VIRTIO_BLK_NO_LEGACY
struct virtio_scsi_inhdr {
__virtio32 errors;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Michael S. Tsirkin @ 2017-07-03 17:03 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <a7e457d5-88c4-745d-5189-18a5f13797dc@redhat.com>
On Wed, Jun 28, 2017 at 08:05:06PM +0800, Jason Wang wrote:
>
>
> On 2017年06月28日 12:01, Michael S. Tsirkin wrote:
> > On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote:
> > >
> > > On 2017年06月28日 11:31, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote:
> > > > > On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
> > > > > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
> > > > > > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS
> > > > > > > > > won't work correctly.
> > > > > > > > >
> > > > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
> > > > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
> > > > > > > > What do you think?
> > > > > > > I think it's safe. For XDP_PASS, it work like in the past.
> > > > > > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
> > > > > > tools assume it's value.
> > > > > DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment
> > > > > in skbuff.h
> > > > >
> > > > >
> > > > > "
> > > > > * The hardware you're dealing with doesn't calculate the full checksum
> > > > > * (as in CHECKSUM_COMPLETE), but it does parse headers and verify
> > > > > checksums
> > > > > * for specific protocols. For such packets it will set
> > > > > CHECKSUM_UNNECESSARY
> > > > > * if their checksums are okay. skb->csum is still undefined in this case
> > > > > * though. A driver or device must never modify the checksum field in the
> > > > > * packet even if checksum is verified.
> > > > > "
> > > > >
> > > > > The csum is correct I believe?
> > > > >
> > > > > Thanks
> > > > That's on input. But I think for tun it's output, where that is equivalent
> > > > to CHECKSUM_NONE
> > > >
> > > >
> > > Yes, but the comment said:
> > >
> > > "
> > > CKSUM_NONE:
> > > *
> > > * The skb was already checksummed by the protocol, or a checksum is not
> > > * required.
> > > *
> > > * CHECKSUM_UNNECESSARY:
> > > *
> > > * This has the same meaning on as CHECKSUM_NONE for checksum offload on
> > > * output.
> > > *
> > > "
> > >
> > > So still correct I think?
> > >
> > > Thanks
> > Hmm maybe I mean NEEDS_CHECKSUM actually.
> >
> > I'll need to re-read the spec.
> >
>
> Not sure this is an issue. But if it is, we can probably checksum the packet
> before passing it to XDP. But it would be a little slow.
>
> Thanks
Right. I confused DATA_VALID with NEEDS_CHECKSUM.
IIUC XDP generally refuses to attach if checksum offload
is enabled.
Could you pls explain how to reproduce the issue you are seeing?
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v1] virtio_blk: Use sysfs_match_string() helper
From: Michael S. Tsirkin @ 2017-07-03 16:37 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jens Axboe, virtualization
In-Reply-To: <1499083530.22624.239.camel@linux.intel.com>
On Mon, Jul 03, 2017 at 03:05:30PM +0300, Andy Shevchenko wrote:
> On Fri, 2017-06-09 at 15:07 +0300, Andy Shevchenko wrote:
> > Use sysfs_match_string() helper instead of open coded variant.
>
> Did I miss maintainer?
>
> >
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
You didn't, I'll merge this in the next PULL.
Thanks!
> > ---
> > drivers/block/virtio_blk.c | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 553cc4c542b4..0e707b8cce9d 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -541,12 +541,9 @@ virtblk_cache_type_store(struct device *dev,
> > struct device_attribute *attr,
> > int i;
> >
> > BUG_ON(!virtio_has_feature(vblk->vdev,
> > VIRTIO_BLK_F_CONFIG_WCE));
> > - for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; )
> > - if (sysfs_streq(buf, virtblk_cache_types[i]))
> > - break;
> > -
> > + i = sysfs_match_string(virtblk_cache_types, buf);
> > if (i < 0)
> > - return -EINVAL;
> > + return i;
> >
> > virtio_cwrite8(vdev, offsetof(struct virtio_blk_config, wce),
> > i);
> > virtblk_update_cache_mode(vdev);
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v1] virtio_blk: Use sysfs_match_string() helper
From: Andy Shevchenko @ 2017-07-03 12:05 UTC (permalink / raw)
To: virtualization, Jens Axboe; +Cc: Michael S. Tsirkin
In-Reply-To: <20170609120742.21959-1-andriy.shevchenko@linux.intel.com>
On Fri, 2017-06-09 at 15:07 +0300, Andy Shevchenko wrote:
> Use sysfs_match_string() helper instead of open coded variant.
Did I miss maintainer?
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/block/virtio_blk.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 553cc4c542b4..0e707b8cce9d 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -541,12 +541,9 @@ virtblk_cache_type_store(struct device *dev,
> struct device_attribute *attr,
> int i;
>
> BUG_ON(!virtio_has_feature(vblk->vdev,
> VIRTIO_BLK_F_CONFIG_WCE));
> - for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; )
> - if (sysfs_streq(buf, virtblk_cache_types[i]))
> - break;
> -
> + i = sysfs_match_string(virtblk_cache_types, buf);
> if (i < 0)
> - return -EINVAL;
> + return i;
>
> virtio_cwrite8(vdev, offsetof(struct virtio_blk_config, wce),
> i);
> virtblk_update_cache_mode(vdev);
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm: ttm: virtio-gpu: dma-buf: Constify ttm_place structures.
From: Gerd Hoffmann @ 2017-07-03 9:48 UTC (permalink / raw)
To: Arvind Yadav, airlied; +Cc: linux-kernel, dri-devel, virtualization
In-Reply-To: <25a189402a516a0142d9a4412da0a597c660a96a.1498981093.git.arvind.yadav.cs@gmail.com>
On Sun, 2017-07-02 at 13:11 +0530, Arvind Yadav wrote:
> ttm_place are not supposed to change at runtime. All functions
> working with ttm_place provided by <drm/ttm/ttm_placement.h> work
> with const ttm_place. So mark the non-const structs as const.
>
> File size before:
> text data bss dec hex
> filename
> 2315 184 0 2499 9c3
> drivers/gpu/drm/virtio/virtgpu_ttm.o
>
> File size After adding 'const':
> text data bss dec hex
> filename
> 2347 152 0 2499 9c3
> drivers/gpu/drm/virtio/virtgpu_ttm.o
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
pushed to drm-misc-next (qxl patch too).
cheers,
Gerd
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] drm: qxl: constify ttm_place structures.
From: Arvind Yadav @ 2017-07-02 17:33 UTC (permalink / raw)
To: airlied, kraxel, airlied; +Cc: linux-kernel, dri-devel, virtualization
ttm_place are not supposed to change at runtime. All functions
working with ttm_place provided by <drm/ttm/ttm_placement.h> work
with const ttm_place. So mark the non-const structs as const.
File size before:
text data bss dec hex filename
3485 184 264 3933 f5d drivers/gpu/drm/qxl/qxl_ttm.o
File size After adding 'const':
text data bss dec hex filename
3501 152 264 3917 f4d drivers/gpu/drm/qxl/qxl_ttm.o
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/gpu/drm/qxl/qxl_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 0fdedee..900fb8a 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -187,7 +187,7 @@ static void qxl_evict_flags(struct ttm_buffer_object *bo,
struct ttm_placement *placement)
{
struct qxl_bo *qbo;
- static struct ttm_place placements = {
+ static const struct ttm_place placements = {
.fpfn = 0,
.lpfn = 0,
.flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM
--
2.7.4
^ permalink raw reply related
* [PATCH] drm: ttm: virtio-gpu: dma-buf: Constify ttm_place structures.
From: Arvind Yadav @ 2017-07-02 7:41 UTC (permalink / raw)
To: airlied, kraxel; +Cc: linux-kernel, dri-devel, virtualization
ttm_place are not supposed to change at runtime. All functions
working with ttm_place provided by <drm/ttm/ttm_placement.h> work
with const ttm_place. So mark the non-const structs as const.
File size before:
text data bss dec hex filename
2315 184 0 2499 9c3 drivers/gpu/drm/virtio/virtgpu_ttm.o
File size After adding 'const':
text data bss dec hex filename
2347 152 0 2499 9c3 drivers/gpu/drm/virtio/virtgpu_ttm.o
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/gpu/drm/virtio/virtgpu_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index 4e8e27d..012d5bc 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -234,7 +234,7 @@ static int virtio_gpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
static void virtio_gpu_evict_flags(struct ttm_buffer_object *bo,
struct ttm_placement *placement)
{
- static struct ttm_place placements = {
+ static const struct ttm_place placements = {
.fpfn = 0,
.lpfn = 0,
.flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM,
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net] virtio-net: serialize tx routine during reset
From: David Miller @ 2017-06-29 16:52 UTC (permalink / raw)
To: jasowang; +Cc: mst, netdev, john.fastabend, linux-kernel, virtualization,
jpmenil
In-Reply-To: <1498614663-7711-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 28 Jun 2017 09:51:03 +0800
> We don't hold any tx lock when trying to disable TX during reset, this
> would lead a use after free since ndo_start_xmit() tries to access
> the virtqueue which has already been freed. Fix this by using
> netif_tx_disable() before freeing the vqs, this could make sure no tx
> after vq freeing.
>
> Reported-by: Jean-Philippe Menil <jpmenil@gmail.com>
> Tested-by: Jean-Philippe Menil <jpmenil@gmail.com>
> Fixes commit f600b6905015 ("virtio_net: Add XDP support")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Matthew Wilcox @ 2017-06-28 15:04 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, amit.shah, kvm, Michael S. Tsirkin,
linux-kernel, liliang.opensource, dave.hansen, qemu-devel,
virtualization, linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <594240E9.2070705@intel.com>
On Thu, Jun 15, 2017 at 04:10:17PM +0800, Wei Wang wrote:
> > So you still have a home-grown bitmap. I'd like to know why
> > isn't xbitmap suggested for this purpose by Matthew Wilcox
> > appropriate. Please add a comment explaining the requirements
> > from the data structure.
>
> I didn't find his xbitmap being upstreamed, did you?
It doesn't have any users in the tree yet. Can't add code with new users.
You should be the first!
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v11 6/6] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Michael S. Tsirkin @ 2017-06-28 15:01 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
riel@redhat.com, kvm@vger.kernel.org, qemu-devel@nongnu.org,
amit.shah@redhat.com, liliang.opensource@gmail.com, Hansen, Dave,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, nilal@redhat.com,
mgorman@techsingularity.net
In-Reply-To: <594B8287.6000706@intel.com>
On Thu, Jun 22, 2017 at 04:40:39PM +0800, Wei Wang wrote:
> On 06/21/2017 08:28 PM, Michael S. Tsirkin wrote:
> > On Wed, Jun 21, 2017 at 11:28:00AM +0800, Wei Wang wrote:
> > > On 06/21/2017 12:18 AM, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 09, 2017 at 06:41:41PM +0800, Wei Wang wrote:
> > > > > - if (!virtqueue_indirect_desc_table_add(vq, desc, num)) {
> > > > > + if (!virtqueue_indirect_desc_table_add(vq, desc, *num)) {
> > > > > virtqueue_kick(vq);
> > > > > - wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > > > - vb->balloon_page_chunk.chunk_num = 0;
> > > > > + if (busy_wait)
> > > > > + while (!virtqueue_get_buf(vq, &len) &&
> > > > > + !virtqueue_is_broken(vq))
> > > > > + cpu_relax();
> > > > > + else
> > > > > + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > > This is something I didn't previously notice.
> > > > As you always keep a single buffer in flight, you do not
> > > > really need indirect at all. Just add all descriptors
> > > > in the ring directly, then kick.
> > > >
> > > > E.g.
> > > > virtqueue_add_first
> > > > virtqueue_add_next
> > > > virtqueue_add_last
> > > >
> > > > ?
> > > >
> > > > You also want a flag to avoid allocations but there's no need to do it
> > > > per descriptor, set it on vq.
> > > >
> > > Without using the indirect table, I'm thinking about changing to use
> > > the standard sg (i.e. struct scatterlist), instead of vring_desc, so that
> > > we don't need to modify or add any new functions of virtqueue_add().
> > >
> > > In this case, we will kmalloc an array of sgs in probe(), and we can add
> > > the sgs one by one to the vq, which won't trigger the allocation of an
> > > indirect table inside virtqueue_add(), and then kick when all are added.
> > >
> > > Best,
> > > Wei
> > And allocate headers too? This can work. API extensions aren't
> > necessarily a bad idea though. The API I suggest above is preferable
> > for the simple reason that it can work without INDIRECT flag
> > support in hypervisor.
>
> OK, probably we don't need to add a desc to the vq - we can just use
> the vq's desc, like this:
>
> int virtqueue_add_first(struct virtqueue *_vq,
> uint64_t addr,
> uint32_t len,
> bool in,
> unsigned int *idx) {
>
> ...
> uint16_t desc_flags = in ? VRING_DESC_F_NEXT | VRING_DESC_F_WRITE :
> VRING_DESC_F_NEXT;
>
> vq->vring.desc[vq->free_head].addr = addr;
> vq->vring.desc[vq->free_head].len = len;
> vq->vring.desc[vq->free_head].flags = cpu_to_virtio16(_vq->vdev, flags);
> /* return to the caller the desc id */
> *idx = vq->free_head;
> ...
> }
>
> int virtqueue_add_next(struct virtqueue *_vq,
> uint64_t addr,
> uint32_t len,
> bool in,
> bool end,
> unsigned int *idx) {
> ...
> vq->vring.desc[*idx].next = vq->free_head;
> vq->vring.desc[vq->free_head].addr = addr;
> ...
> if (end)
> remove the VRING_DESC_F_NEXT flag
> }
>
Add I would say add-last.
>
> What do you think? We can also combine the two functions into one.
>
>
>
> Best,
> Wei
With an enum? Yes that's also an option.
--
MST
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Jason Wang @ 2017-06-28 12:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170628065126-mutt-send-email-mst@kernel.org>
On 2017年06月28日 12:01, Michael S. Tsirkin wrote:
> On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote:
>>
>> On 2017年06月28日 11:31, Michael S. Tsirkin wrote:
>>> On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote:
>>>> On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
>>>>>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
>>>>>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS
>>>>>>>> won't work correctly.
>>>>>>>>
>>>>>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
>>>>>>> What do you think?
>>>>>> I think it's safe. For XDP_PASS, it work like in the past.
>>>>> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
>>>>> tools assume it's value.
>>>> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment
>>>> in skbuff.h
>>>>
>>>>
>>>> "
>>>> * The hardware you're dealing with doesn't calculate the full checksum
>>>> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify
>>>> checksums
>>>> * for specific protocols. For such packets it will set
>>>> CHECKSUM_UNNECESSARY
>>>> * if their checksums are okay. skb->csum is still undefined in this case
>>>> * though. A driver or device must never modify the checksum field in the
>>>> * packet even if checksum is verified.
>>>> "
>>>>
>>>> The csum is correct I believe?
>>>>
>>>> Thanks
>>> That's on input. But I think for tun it's output, where that is equivalent
>>> to CHECKSUM_NONE
>>>
>>>
>> Yes, but the comment said:
>>
>> "
>> CKSUM_NONE:
>> *
>> * The skb was already checksummed by the protocol, or a checksum is not
>> * required.
>> *
>> * CHECKSUM_UNNECESSARY:
>> *
>> * This has the same meaning on as CHECKSUM_NONE for checksum offload on
>> * output.
>> *
>> "
>>
>> So still correct I think?
>>
>> Thanks
> Hmm maybe I mean NEEDS_CHECKSUM actually.
>
> I'll need to re-read the spec.
>
Not sure this is an issue. But if it is, we can probably checksum the
packet before passing it to XDP. But it would be a little slow.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Michael S. Tsirkin @ 2017-06-28 4:01 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <51f4c402-6a02-9bad-6dab-563ca72f431a@redhat.com>
On Wed, Jun 28, 2017 at 11:40:30AM +0800, Jason Wang wrote:
>
>
> On 2017年06月28日 11:31, Michael S. Tsirkin wrote:
> > On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote:
> > > On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
> > > > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
> > > > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS
> > > > > > > won't work correctly.
> > > > > > >
> > > > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
> > > > > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
> > > > > > What do you think?
> > > > > I think it's safe. For XDP_PASS, it work like in the past.
> > > > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
> > > > tools assume it's value.
> > > DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment
> > > in skbuff.h
> > >
> > >
> > > "
> > > * The hardware you're dealing with doesn't calculate the full checksum
> > > * (as in CHECKSUM_COMPLETE), but it does parse headers and verify
> > > checksums
> > > * for specific protocols. For such packets it will set
> > > CHECKSUM_UNNECESSARY
> > > * if their checksums are okay. skb->csum is still undefined in this case
> > > * though. A driver or device must never modify the checksum field in the
> > > * packet even if checksum is verified.
> > > "
> > >
> > > The csum is correct I believe?
> > >
> > > Thanks
> > That's on input. But I think for tun it's output, where that is equivalent
> > to CHECKSUM_NONE
> >
> >
>
> Yes, but the comment said:
>
> "
> CKSUM_NONE:
> *
> * The skb was already checksummed by the protocol, or a checksum is not
> * required.
> *
> * CHECKSUM_UNNECESSARY:
> *
> * This has the same meaning on as CHECKSUM_NONE for checksum offload on
> * output.
> *
> "
>
> So still correct I think?
>
> Thanks
Hmm maybe I mean NEEDS_CHECKSUM actually.
I'll need to re-read the spec.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Jason Wang @ 2017-06-28 3:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170628063033-mutt-send-email-mst@kernel.org>
On 2017年06月28日 11:31, Michael S. Tsirkin wrote:
> On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote:
>> On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
>>> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
>>>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
>>>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
>>>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS
>>>>>> won't work correctly.
>>>>>>
>>>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
>>>>> What do you think?
>>>> I think it's safe. For XDP_PASS, it work like in the past.
>>> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
>>> tools assume it's value.
>> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment
>> in skbuff.h
>>
>>
>> "
>> * The hardware you're dealing with doesn't calculate the full checksum
>> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify
>> checksums
>> * for specific protocols. For such packets it will set
>> CHECKSUM_UNNECESSARY
>> * if their checksums are okay. skb->csum is still undefined in this case
>> * though. A driver or device must never modify the checksum field in the
>> * packet even if checksum is verified.
>> "
>>
>> The csum is correct I believe?
>>
>> Thanks
> That's on input. But I think for tun it's output, where that is equivalent
> to CHECKSUM_NONE
>
>
Yes, but the comment said:
"
CKSUM_NONE:
*
* The skb was already checksummed by the protocol, or a checksum is not
* required.
*
* CHECKSUM_UNNECESSARY:
*
* This has the same meaning on as CHECKSUM_NONE for checksum offload on
* output.
*
"
So still correct I think?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Michael S. Tsirkin @ 2017-06-28 3:31 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <69907228-fc1b-175f-f6cd-7ac332e318be@redhat.com>
On Wed, Jun 28, 2017 at 10:45:18AM +0800, Jason Wang wrote:
>
>
> On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
> > On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
> > >
> > > On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
> > > > > We should allow csumed packet for small buffer, otherwise XDP_PASS
> > > > > won't work correctly.
> > > > >
> > > > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
> > > > What do you think?
> > > I think it's safe. For XDP_PASS, it work like in the past.
> > That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
> > tools assume it's value.
>
> DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the comment
> in skbuff.h
>
>
> "
> * The hardware you're dealing with doesn't calculate the full checksum
> * (as in CHECKSUM_COMPLETE), but it does parse headers and verify
> checksums
> * for specific protocols. For such packets it will set
> CHECKSUM_UNNECESSARY
> * if their checksums are okay. skb->csum is still undefined in this case
> * though. A driver or device must never modify the checksum field in the
> * packet even if checksum is verified.
> "
>
> The csum is correct I believe?
>
> Thanks
That's on input. But I think for tun it's output, where that is equivalent
to CHECKSUM_NONE
> >
> > > For XDP_TX, we
> > > zero the vnet header.
> > Again TX offload is disabled, so packets will go out with an invalid
> > checksum.
> >
> > > For adjusting header, XDP prog should deal with csum.
> > >
> > > Thanks
> > That part seems right.
> >
> > > > > ---
> > > > > The patch is needed for -stable.
> > > > > ---
> > > > > drivers/net/virtio_net.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 143d8a9..499fcc9 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> > > > > void *orig_data;
> > > > > u32 act;
> > > > > - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
> > > > > + if (unlikely(hdr->hdr.gso_type))
> > > > > goto err_xdp;
> > > > > xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> > > > > --
> > > > > 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Jason Wang @ 2017-06-28 2:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170628051555-mutt-send-email-mst@kernel.org>
On 2017年06月28日 10:17, Michael S. Tsirkin wrote:
> On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
>>
>> On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
>>> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
>>>> We should allow csumed packet for small buffer, otherwise XDP_PASS
>>>> won't work correctly.
>>>>
>>>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
>>> What do you think?
>> I think it's safe. For XDP_PASS, it work like in the past.
> That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
> tools assume it's value.
DATA_VALID is CHECKSUM_UNCESSARY on the host, and according to the
comment in skbuff.h
"
* The hardware you're dealing with doesn't calculate the full checksum
* (as in CHECKSUM_COMPLETE), but it does parse headers and verify
checksums
* for specific protocols. For such packets it will set
CHECKSUM_UNNECESSARY
* if their checksums are okay. skb->csum is still undefined in this case
* though. A driver or device must never modify the checksum field in the
* packet even if checksum is verified.
"
The csum is correct I believe?
Thanks
>
>> For XDP_TX, we
>> zero the vnet header.
> Again TX offload is disabled, so packets will go out with an invalid
> checksum.
>
>> For adjusting header, XDP prog should deal with csum.
>>
>> Thanks
> That part seems right.
>
>>>> ---
>>>> The patch is needed for -stable.
>>>> ---
>>>> drivers/net/virtio_net.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 143d8a9..499fcc9 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>>> void *orig_data;
>>>> u32 act;
>>>> - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
>>>> + if (unlikely(hdr->hdr.gso_type))
>>>> goto err_xdp;
>>>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>>>> --
>>>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Michael S. Tsirkin @ 2017-06-28 2:17 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <7068053c-50da-6779-5ff2-6588e01e616d@redhat.com>
On Wed, Jun 28, 2017 at 10:14:34AM +0800, Jason Wang wrote:
>
>
> On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
> > On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
> > > We should allow csumed packet for small buffer, otherwise XDP_PASS
> > > won't work correctly.
> > >
> > > Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
> > What do you think?
>
> I think it's safe. For XDP_PASS, it work like in the past.
That's the part I don't get. With DATA_VALID csum in packet is wrong, XDP
tools assume it's value.
> For XDP_TX, we
> zero the vnet header.
Again TX offload is disabled, so packets will go out with an invalid
checksum.
> For adjusting header, XDP prog should deal with csum.
>
> Thanks
That part seems right.
> >
> > > ---
> > > The patch is needed for -stable.
> > > ---
> > > drivers/net/virtio_net.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 143d8a9..499fcc9 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> > > void *orig_data;
> > > u32 act;
> > > - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
> > > + if (unlikely(hdr->hdr.gso_type))
> > > goto err_xdp;
> > > xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> > > --
> > > 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Jason Wang @ 2017-06-28 2:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170628050057-mutt-send-email-mst@kernel.org>
On 2017年06月28日 10:02, Michael S. Tsirkin wrote:
> On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
>> We should allow csumed packet for small buffer, otherwise XDP_PASS
>> won't work correctly.
>>
>> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
> What do you think?
I think it's safe. For XDP_PASS, it work like in the past. For XDP_TX,
we zero the vnet header. For adjusting header, XDP prog should deal with
csum.
Thanks
>
>> ---
>> The patch is needed for -stable.
>> ---
>> drivers/net/virtio_net.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 143d8a9..499fcc9 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> void *orig_data;
>> u32 act;
>>
>> - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
>> + if (unlikely(hdr->hdr.gso_type))
>> goto err_xdp;
>>
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Michael S. Tsirkin @ 2017-06-28 2:02 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1498614843-8163-1-git-send-email-jasowang@redhat.com>
On Wed, Jun 28, 2017 at 09:54:03AM +0800, Jason Wang wrote:
> We should allow csumed packet for small buffer, otherwise XDP_PASS
> won't work correctly.
>
> Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
The issue would be VIRTIO_NET_HDR_F_DATA_VALID might be set.
What do you think?
> ---
> The patch is needed for -stable.
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 143d8a9..499fcc9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> void *orig_data;
> u32 act;
>
> - if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
> + if (unlikely(hdr->hdr.gso_type))
> goto err_xdp;
>
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net] virtio-net: serialize tx routine during reset
From: Michael S. Tsirkin @ 2017-06-28 2:00 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, jpmenil, John Fastabend, linux-kernel, virtualization
In-Reply-To: <1498614663-7711-1-git-send-email-jasowang@redhat.com>
On Wed, Jun 28, 2017 at 09:51:03AM +0800, Jason Wang wrote:
> We don't hold any tx lock when trying to disable TX during reset, this
> would lead a use after free since ndo_start_xmit() tries to access
> the virtqueue which has already been freed. Fix this by using
> netif_tx_disable() before freeing the vqs, this could make sure no tx
> after vq freeing.
>
> Reported-by: Jean-Philippe Menil <jpmenil@gmail.com>
> Tested-by: Jean-Philippe Menil <jpmenil@gmail.com>
> Fixes commit f600b6905015 ("virtio_net: Add XDP support")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Thanks a lot Jason. I think this is needed in stable as well.
> ---
> drivers/net/virtio_net.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a871f45..143d8a9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1797,6 +1797,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
> flush_work(&vi->config_work);
>
> netif_device_detach(vi->dev);
> + netif_tx_disable(vi->dev);
> cancel_delayed_work_sync(&vi->refill);
>
> if (netif_running(vi->dev)) {
> --
> 2.7.4
^ permalink raw reply
* [PATCH net] virtio-net: unbreak cusmed packet for small buffer XDP
From: Jason Wang @ 2017-06-28 1:54 UTC (permalink / raw)
To: mst, virtualization, netdev, linux-kernel
We should allow csumed packet for small buffer, otherwise XDP_PASS
won't work correctly.
Fixes commit bb91accf2733 ("virtio-net: XDP support for small buffers")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
The patch is needed for -stable.
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 143d8a9..499fcc9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -413,7 +413,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
void *orig_data;
u32 act;
- if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
+ if (unlikely(hdr->hdr.gso_type))
goto err_xdp;
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
--
2.7.4
^ permalink raw reply related
* [PATCH net] virtio-net: serialize tx routine during reset
From: Jason Wang @ 2017-06-28 1:51 UTC (permalink / raw)
To: mst, virtualization, netdev, linux-kernel; +Cc: John Fastabend, jpmenil
We don't hold any tx lock when trying to disable TX during reset, this
would lead a use after free since ndo_start_xmit() tries to access
the virtqueue which has already been freed. Fix this by using
netif_tx_disable() before freeing the vqs, this could make sure no tx
after vq freeing.
Reported-by: Jean-Philippe Menil <jpmenil@gmail.com>
Tested-by: Jean-Philippe Menil <jpmenil@gmail.com>
Fixes commit f600b6905015 ("virtio_net: Add XDP support")
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a871f45..143d8a9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1797,6 +1797,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
flush_work(&vi->config_work);
netif_device_detach(vi->dev);
+ netif_tx_disable(vi->dev);
cancel_delayed_work_sync(&vi->refill);
if (netif_running(vi->dev)) {
--
2.7.4
^ permalink raw reply related
* Re: [Qemu-devel] BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jean-Philippe Menil @ 2017-06-27 12:35 UTC (permalink / raw)
To: Jason Wang, Cong Wang
Cc: Linux Kernel Network Developers, Michael S. Tsirkin,
John Fastabend, qemu-devel Developers, virtualization
In-Reply-To: <56868f1a-b4d9-50ca-873b-8fe4f70846d6@redhat.com>
On 06/27/2017 04:13 AM, Jason Wang wrote:
>
>
> On 2017年06月26日 15:35, Jean-Philippe Menil wrote:
>> On 06/26/2017 04:50 AM, Jason Wang wrote:
>>>
>>>
>>> On 2017年06月24日 06:32, Cong Wang wrote:
>>>> On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com>
>>>> wrote:
>>>>>
>>>>> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>>>>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>>>>> Hi Michael,
>>>>>>>
>>>>>>> from what i see, the race appear when we hit virtnet_reset in
>>>>>>> virtnet_xdp_set.
>>>>>>> virtnet_reset
>>>>>>> _remove_vq_common
>>>>>>> virtnet_del_vqs
>>>>>>> virtnet_free_queues
>>>>>>> kfree(vi->sq)
>>>>>>> when the xdp program (with two instances of the program to
>>>>>>> trigger it
>>>>>>> faster)
>>>>>>> is added or removed.
>>>>>>>
>>>>>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>>>>>> line,
>>>>>>> running the xdp_ttl tool from Jesper.
>>>>>>>
>>>>>>> For now, i'm able to continue my qualification, testing if xdp_qp
>>>>>>> is not
>>>>>>> null,
>>>>>>> but do not seem to be a sustainable trick.
>>>>>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>>>>>
>>>>>>> Maybe it will be more clear to you with theses informations.
>>>>>>>
>>>>>>> Best regards.
>>>>>>>
>>>>>>> Jean-Philippe
>>>>>>
>>>>>> I'm pretty clear about the issue here, I was trying to figure out
>>>>>> a fix.
>>>>>> Jason, any thoughts?
>>>>>>
>>>>>>
>>>>> Hi Jean:
>>>>>
>>>>> Does the following fix this issue? (I can't reproduce it locally
>>>>> through
>>>>> xdp_ttl)
>>>> It is tricky here.
>>>>
>>>> From my understanding of the code base, the tx_lock is not sufficient
>>>> here, because in virtnet_del_vqs() all vqs are deleted and one vp
>>>> maps to one txq.
>>>>
>>>> I am afraid you have to add a spinlock somewhere to serialized
>>>> free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
>>>> they are in different layers, so it is hard to figure out where to add
>>>> it...
>>>>
>>>> Also, make sure we don't sleep inside the spinlock, I see a
>>>> synchronize_net().
>>>
>>> Looks like I miss something. I thought free_old_xmit_skbs() were
>>> serialized in this case since we disable all tx queues after
>>> netif_tx_unlock_bh()?
>>>
>>> Jean:
>>>
>>> I thought this could be easily reproduced by e.g produce some traffic
>>> and in the same time try to attach an xdp program. But looks not. How
>>> do you trigger this? What's your qemu command line for this?
>>>
>>> Thanks
>>
>> Hi Jason,
>>
>> this is how i trigger the bug:
>> - on the guest, tcpdump on on the interface
>> - on the guest, run iperf against the host
>> - on the guest, cat /sys/kernel/debug/tracing/trace_pipe
>> - on the guest, run one or two instances of xdp_ttl compiled with
>> DEBUG uncommented, that i start stop, until i trigger the bug.
>>
>> qemu command line is as follow:
>>
>> qemu-system-x86_64 -name ubuntu --enable-kvm -machine pc,accel=kvm
>> -smp 2 -drive file=/dev/LocalDisk/ubuntu,if=virtio,format=raw -m 2048
>> -rtc base=localtime,clock=host -usbdevice tablet --balloon virtio
>> -netdev
>> tap,id=ubuntu-0,ifname=ubuntu-0,script=/home/jenfi/WORK/jp/qemu/if-up,downscript=/home/jenfi/WORK/jp/qemu/if-down,vhost=on,queues=4
>> -device
>> virtio-net-pci,netdev=ubuntu-0,mac=de:ad:be:ef:01:03,mq=on,guest_tso4=off,guest_tso6=off,guest_ecn=off,guest_ufo=off,vectors=2
>> -vnc 127.0.0.1:3 -nographic -serial
>> file:/home/jenfi/WORK/jp/qemu/ubuntu.out -monitor
>> unix:/home/jenfi/WORK/jp/qemu/ubuntu.sock,server,nowait
>>
>> Notice, the smp 2, queues to 4 and vectors to 2.
>> Seem that if fogot to mention that in the beginning of this thread,
>> sorry for that.
>>
>> Best regards.
>>
>> Jean-Philippe
>>
>
> Thanks Jean, I manage to reproduce the issue.
>
> I thought netif_tx_unlock_bh() will do tx lock but looks not, that's why
> previous patch doesn't work.
>
> Could you please this this patch? (At least it can't trigger the warning
> after more than 20 times of xdp start/stop).
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1f8c15c..a18f859 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1802,6 +1802,7 @@ static void virtnet_freeze_down(struct
> virtio_device *vdev)
> flush_work(&vi->config_work);
>
> netif_device_detach(vi->dev);
> + netif_tx_disable(vi->dev);
> cancel_delayed_work_sync(&vi->refill);
>
> if (netif_running(vi->dev)) {
>
>
Hi Jason,
Seem to do the trick !
with your patch, i'm unable to repeat the problem anymore (running more
than 2h without any issue).
Best regards.
Jean-Philippe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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