| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230324051153-mutt-send-email-mst@kernel.org>
Date: Fri, 24 Mar 2023 05:12:26 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Xie Yongji <xieyongji@...edance.com>, tglx@...utronix.de,
hch@....de, virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 03/11] virtio-vdpa: Support interrupt affinity
spreading mechanism
On Fri, Mar 24, 2023 at 02:27:52PM +0800, Jason Wang wrote:
> On Thu, Mar 23, 2023 at 1:31 PM Xie Yongji <xieyongji@...edance.com> wrote:
> >
> > To support interrupt affinity spreading mechanism,
> > this makes use of group_cpus_evenly() to create
> > an irq callback affinity mask for each virtqueue
> > of vdpa device. Then we will unify set_vq_affinity
> > callback to pass the affinity to the vdpa device driver.
> >
> > Signed-off-by: Xie Yongji <xieyongji@...edance.com>
>
> Thinking hard of all the logics, I think I've found something interesting.
>
> Commit ad71473d9c437 ("virtio_blk: use virtio IRQ affinity") tries to
> pass irq_affinity to transport specific find_vqs(). This seems a
> layer violation since driver has no knowledge of
>
> 1) whether or not the callback is based on an IRQ
> 2) whether or not the device is a PCI or not (the details are hided by
> the transport driver)
> 3) how many vectors could be used by a device
>
> This means the driver can't actually pass a real affinity masks so the
> commit passes a zero irq affinity structure as a hint in fact, so the
> PCI layer can build a default affinity based that groups cpus evenly
> based on the number of MSI-X vectors (the core logic is the
> group_cpus_evenly). I think we should fix this by replacing the
> irq_affinity structure with
>
> 1) a boolean like auto_cb_spreading
>
> or
>
> 2) queue to cpu mapping
>
> So each transport can do its own logic based on that. Then virtio-vDPA
> can pass that policy to VDUSE where we only need a group_cpus_evenly()
> and avoid duplicating irq_create_affinity_masks()?
>
> Thanks
I don't really understand what you propose. Care to post a patch?
Also does it have to block this patchset or can it be done on top?
> > ---
> > drivers/virtio/virtio_vdpa.c | 68 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 68 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> > index f72696b4c1c2..f3826f42b704 100644
> > --- a/drivers/virtio/virtio_vdpa.c
> > +++ b/drivers/virtio/virtio_vdpa.c
> > @@ -13,6 +13,7 @@
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > #include <linux/uuid.h>
> > +#include <linux/group_cpus.h>
> > #include <linux/virtio.h>
> > #include <linux/vdpa.h>
> > #include <linux/virtio_config.h>
> > @@ -272,6 +273,66 @@ static void virtio_vdpa_del_vqs(struct virtio_device *vdev)
> > virtio_vdpa_del_vq(vq);
> > }
> >
> > +static void default_calc_sets(struct irq_affinity *affd, unsigned int affvecs)
> > +{
> > + affd->nr_sets = 1;
> > + affd->set_size[0] = affvecs;
> > +}
> > +
> > +static struct cpumask *
> > +create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
> > +{
> > + unsigned int affvecs = 0, curvec, usedvecs, i;
> > + struct cpumask *masks = NULL;
> > +
> > + if (nvecs > affd->pre_vectors + affd->post_vectors)
> > + affvecs = nvecs - affd->pre_vectors - affd->post_vectors;
> > +
> > + if (!affd->calc_sets)
> > + affd->calc_sets = default_calc_sets;
> > +
> > + affd->calc_sets(affd, affvecs);
> > +
> > + if (!affvecs)
> > + return NULL;
> > +
> > + masks = kcalloc(nvecs, sizeof(*masks), GFP_KERNEL);
> > + if (!masks)
> > + return NULL;
> > +
> > + /* Fill out vectors at the beginning that don't need affinity */
> > + for (curvec = 0; curvec < affd->pre_vectors; curvec++)
> > + cpumask_setall(&masks[curvec]);
> > +
> > + for (i = 0, usedvecs = 0; i < affd->nr_sets; i++) {
> > + unsigned int this_vecs = affd->set_size[i];
> > + int j;
> > + struct cpumask *result = group_cpus_evenly(this_vecs);
> > +
> > + if (!result) {
> > + kfree(masks);
> > + return NULL;
> > + }
> > +
> > + for (j = 0; j < this_vecs; j++)
> > + cpumask_copy(&masks[curvec + j], &result[j]);
> > + kfree(result);
> > +
> > + curvec += this_vecs;
> > + usedvecs += this_vecs;
> > + }
> > +
> > + /* Fill out vectors at the end that don't need affinity */
> > + if (usedvecs >= affvecs)
> > + curvec = affd->pre_vectors + affvecs;
> > + else
> > + curvec = affd->pre_vectors + usedvecs;
> > + for (; curvec < nvecs; curvec++)
> > + cpumask_setall(&masks[curvec]);
> > +
> > + return masks;
> > +}
> > +
> > static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> > struct virtqueue *vqs[],
> > vq_callback_t *callbacks[],
> > @@ -282,9 +343,15 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> > struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
> > struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> > const struct vdpa_config_ops *ops = vdpa->config;
> > + struct irq_affinity default_affd = { 0 };
> > + struct cpumask *masks;
> > struct vdpa_callback cb;
> > int i, err, queue_idx = 0;
> >
> > + masks = create_affinity_masks(nvqs, desc ? desc : &default_affd);
> > + if (!masks)
> > + return -ENOMEM;
> > +
> > for (i = 0; i < nvqs; ++i) {
> > if (!names[i]) {
> > vqs[i] = NULL;
> > @@ -298,6 +365,7 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> > err = PTR_ERR(vqs[i]);
> > goto err_setup_vq;
> > }
> > + ops->set_vq_affinity(vdpa, i, &masks[i]);
> > }
> >
> > cb.callback = virtio_vdpa_config_cb;
> > --
> > 2.20.1
> >
Powered by blists - more mailing lists