MIME-Version: 1.0
In-Reply-To: <1337515228-31910-1-git-send-email-ohad@wizery.com>
References: <1337515228-31910-1-git-send-email-ohad@wizery.com>
Date: Mon, 21 May 2012 11:49:10 -0500
Message-ID: <CACY--9FBeNHDVMtLC09OtMm=t3ZFw2Gtipn+8ojAEqHAJebSnQ@mail.gmail.com>
Subject: Re: [PATCH] remoteproc: allocate vrings on demand, free when not needed
From: "Guzman Lugo, Fernando" <fernando.lugo@ti.com>
To: Ohad Ben-Cohen <ohad@wizery.com>
Cc: linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, Michal Simek <monstr@monstr.eu>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 11994
Lines: 332

Looks good to me.

 Tested-by: Fernando Guzman Lugo <fernando.lugo@ti.com>

Regards,
Fernando.


On Sun, May 20, 2012 at 7:00 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>
> Dynamically allocate the vrings' DMA when the remote processor
> is about to be powered on (i.e. when ->find_vqs() is invoked),
> and release them as soon as it is powered off (i.e. when ->del_vqs()
> is invoked).
>
> The obvious and immediate benefit is better memory utilization, since
> memory for the vrings is now only allocated when the relevant remote
> processor is being used.
>
> Additionally, this approach also makes recovery of a (crashing)
> remote processor easier: one just needs to remove the relevant
> vdevs, and the entire vrings cleanup takes place automagically.
>
> Tested-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
> ?drivers/remoteproc/remoteproc_core.c ? ? | ?109
> +++++++++++++++---------------
> ?drivers/remoteproc/remoteproc_internal.h | ? ?2 +
> ?drivers/remoteproc/remoteproc_virtio.c ? | ? 13 +++-
> ?3 files changed, 67 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index e756a0d..40e2b2d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -279,34 +279,17 @@ rproc_load_segments(struct rproc *rproc, const u8
> *elf_data, size_t len)
> ? ? ? ?return ret;
> ?}
>
> -static int
> -__rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc,
> int i)
> +int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
> ?{
> ? ? ? ?struct rproc *rproc = rvdev->rproc;
> ? ? ? ?struct device *dev = rproc->dev;
> - ? ? ? struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
> + ? ? ? struct rproc_vring *rvring = &rvdev->vring[i];
> ? ? ? ?dma_addr_t dma;
> ? ? ? ?void *va;
> ? ? ? ?int ret, size, notifyid;
>
> - ? ? ? dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i, vring->da, vring->num, vring->align);
> -
> - ? ? ? /* make sure reserved bytes are zeroes */
> - ? ? ? if (vring->reserved) {
> - ? ? ? ? ? ? ? dev_err(dev, "vring rsc has non zero reserved bytes\n");
> - ? ? ? ? ? ? ? return -EINVAL;
> - ? ? ? }
> -
> - ? ? ? /* verify queue size and vring alignment are sane */
> - ? ? ? if (!vring->num || !vring->align) {
> - ? ? ? ? ? ? ? dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? vring->num, vring->align);
> - ? ? ? ? ? ? ? return -EINVAL;
> - ? ? ? }
> -
> ? ? ? ?/* actual size of vring (in bytes) */
> - ? ? ? size = PAGE_ALIGN(vring_size(vring->num, vring->align));
> + ? ? ? size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>
> ? ? ? ?if (!idr_pre_get(&rproc->notifyids, GFP_KERNEL)) {
> ? ? ? ? ? ? ? ?dev_err(dev, "idr_pre_get failed\n");
> @@ -316,6 +299,7 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct
> fw_rsc_vdev *rsc, int i)
> ? ? ? ?/*
> ? ? ? ? * Allocate non-cacheable memory for the vring. In the future
> ? ? ? ? * this call will also configure the IOMMU for us
> + ? ? ? ?* TODO: let the rproc know the da of this vring
> ? ? ? ? */
> ? ? ? ?va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
> ? ? ? ?if (!va) {
> @@ -323,44 +307,67 @@ __rproc_handle_vring(struct rproc_vdev *rvdev,
> struct fw_rsc_vdev *rsc, int i)
> ? ? ? ? ? ? ? ?return -EINVAL;
> ? ? ? ?}
>
> - ? ? ? /* assign an rproc-wide unique index for this vring */
> - ? ? ? /* TODO: assign a notifyid for rvdev updates as well */
> - ? ? ? ret = idr_get_new(&rproc->notifyids, &rvdev->vring[i], &notifyid);
> + ? ? ? /*
> + ? ? ? ?* Assign an rproc-wide unique index for this vring
> + ? ? ? ?* TODO: assign a notifyid for rvdev updates as well
> + ? ? ? ?* TODO: let the rproc know the notifyid of this vring
> + ? ? ? ?* TODO: support predefined notifyids (via resource table)
> + ? ? ? ?*/
> + ? ? ? ret = idr_get_new(&rproc->notifyids, rvring, &notifyid);
> ? ? ? ?if (ret) {
> ? ? ? ? ? ? ? ?dev_err(dev, "idr_get_new failed: %d\n", ret);
> ? ? ? ? ? ? ? ?dma_free_coherent(dev, size, va, dma);
> ? ? ? ? ? ? ? ?return ret;
> ? ? ? ?}
>
> - ? ? ? /* let the rproc know the da and notifyid of this vring */
> - ? ? ? /* TODO: expose this to remote processor */
> - ? ? ? vring->da = dma;
> - ? ? ? vring->notifyid = notifyid;
> -
> ? ? ? ?dev_dbg(dev, "vring%d: va %p dma %x size %x idr %d\n", i, va,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?dma, size, notifyid);
>
> - ? ? ? rvdev->vring[i].len = vring->num;
> - ? ? ? rvdev->vring[i].align = vring->align;
> - ? ? ? rvdev->vring[i].va = va;
> - ? ? ? rvdev->vring[i].dma = dma;
> - ? ? ? rvdev->vring[i].notifyid = notifyid;
> - ? ? ? rvdev->vring[i].rvdev = rvdev;
> + ? ? ? rvring->va = va;
> + ? ? ? rvring->dma = dma;
> + ? ? ? rvring->notifyid = notifyid;
>
> ? ? ? ?return 0;
> ?}
>
> -static void __rproc_free_vrings(struct rproc_vdev *rvdev, int i)
> +static int
> +rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int
> i)
> ?{
> ? ? ? ?struct rproc *rproc = rvdev->rproc;
> + ? ? ? struct device *dev = rproc->dev;
> + ? ? ? struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
> + ? ? ? struct rproc_vring *rvring = &rvdev->vring[i];
>
> - ? ? ? for (i--; i >= 0; i--) {
> - ? ? ? ? ? ? ? struct rproc_vring *rvring = &rvdev->vring[i];
> - ? ? ? ? ? ? ? int size = PAGE_ALIGN(vring_size(rvring->len,
> rvring->align));
> + ? ? ? dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i, vring->da, vring->num, vring->align);
> +
> + ? ? ? /* make sure reserved bytes are zeroes */
> + ? ? ? if (vring->reserved) {
> + ? ? ? ? ? ? ? dev_err(dev, "vring rsc has non zero reserved bytes\n");
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
>
> - ? ? ? ? ? ? ? dma_free_coherent(rproc->dev, size, rvring->va,
> rvring->dma);
> - ? ? ? ? ? ? ? idr_remove(&rproc->notifyids, rvring->notifyid);
> + ? ? ? /* verify queue size and vring alignment are sane */
> + ? ? ? if (!vring->num || !vring->align) {
> + ? ? ? ? ? ? ? dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? vring->num, vring->align);
> + ? ? ? ? ? ? ? return -EINVAL;
> ? ? ? ?}
> +
> + ? ? ? rvring->len = vring->num;
> + ? ? ? rvring->align = vring->align;
> + ? ? ? rvring->rvdev = rvdev;
> +
> + ? ? ? return 0;
> +}
> +
> +void rproc_free_vring(struct rproc_vring *rvring)
> +{
> + ? ? ? int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
> + ? ? ? struct rproc *rproc = rvring->rvdev->rproc;
> +
> + ? ? ? dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
> + ? ? ? idr_remove(&rproc->notifyids, rvring->notifyid);
> ?}
>
> ?/**
> @@ -425,11 +432,11 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
>
> ? ? ? ?rvdev->rproc = rproc;
>
> - ? ? ? /* allocate the vrings */
> + ? ? ? /* parse the vrings */
> ? ? ? ?for (i = 0; i < rsc->num_of_vrings; i++) {
> - ? ? ? ? ? ? ? ret = __rproc_handle_vring(rvdev, rsc, i);
> + ? ? ? ? ? ? ? ret = rproc_parse_vring(rvdev, rsc, i);
> ? ? ? ? ? ? ? ?if (ret)
> - ? ? ? ? ? ? ? ? ? ? ? goto free_vrings;
> + ? ? ? ? ? ? ? ? ? ? ? goto free_rvdev;
> ? ? ? ?}
>
> ? ? ? ?/* remember the device features */
> @@ -440,12 +447,11 @@ static int rproc_handle_vdev(struct rproc *rproc,
> struct fw_rsc_vdev *rsc,
> ? ? ? ?/* it is now safe to add the virtio device */
> ? ? ? ?ret = rproc_add_virtio_dev(rvdev, rsc->id);
> ? ? ? ?if (ret)
> - ? ? ? ? ? ? ? goto free_vrings;
> + ? ? ? ? ? ? ? goto free_rvdev;
>
> ? ? ? ?return 0;
>
> -free_vrings:
> - ? ? ? __rproc_free_vrings(rvdev, i);
> +free_rvdev:
> ? ? ? ?kfree(rvdev);
> ? ? ? ?return ret;
> ?}
> @@ -1265,18 +1271,11 @@ EXPORT_SYMBOL(rproc_shutdown);
> ?void rproc_release(struct kref *kref)
> ?{
> ? ? ? ?struct rproc *rproc = container_of(kref, struct rproc, refcount);
> - ? ? ? struct rproc_vdev *rvdev, *rvtmp;
>
> ? ? ? ?dev_info(rproc->dev, "removing %s\n", rproc->name);
>
> ? ? ? ?rproc_delete_debug_dir(rproc);
>
> - ? ? ? /* clean up remote vdev entries */
> - ? ? ? list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node) {
> - ? ? ? ? ? ? ? __rproc_free_vrings(rvdev, RVDEV_NUM_VRINGS);
> - ? ? ? ? ? ? ? list_del(&rvdev->node);
> - ? ? ? }
> -
> ? ? ? ?/*
> ? ? ? ? * At this point no one holds a reference to rproc anymore,
> ? ? ? ? * so we can directly unroll rproc_alloc()
> @@ -1547,7 +1546,7 @@ EXPORT_SYMBOL(rproc_free);
> ?*/
> ?int rproc_unregister(struct rproc *rproc)
> ?{
> - ? ? ? struct rproc_vdev *rvdev;
> + ? ? ? struct rproc_vdev *rvdev, *tmp;
>
> ? ? ? ?if (!rproc)
> ? ? ? ? ? ? ? ?return -EINVAL;
> @@ -1556,7 +1555,7 @@ int rproc_unregister(struct rproc *rproc)
> ? ? ? ?wait_for_completion(&rproc->firmware_loading_complete);
>
> ? ? ? ?/* clean up remote vdev entries */
> - ? ? ? list_for_each_entry(rvdev, &rproc->rvdevs, node)
> + ? ? ? list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
> ? ? ? ? ? ? ? ?rproc_remove_virtio_dev(rvdev);
>
> ? ? ? ?/* the rproc is downref'ed as soon as it's removed from the klist
> */
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index 9f336d6..f4957cf 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -41,4 +41,6 @@ void rproc_create_debug_dir(struct rproc *rproc);
> ?void rproc_init_debugfs(void);
> ?void rproc_exit_debugfs(void);
>
> +void rproc_free_vring(struct rproc_vring *rvring);
> +int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
> ?#endif /* REMOTEPROC_INTERNAL_H */
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> index ecf6121..26a7144 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -77,14 +77,17 @@ static struct virtqueue *rp_find_vq(struct
> virtio_device *vdev,
> ? ? ? ?struct rproc_vring *rvring;
> ? ? ? ?struct virtqueue *vq;
> ? ? ? ?void *addr;
> - ? ? ? int len, size;
> + ? ? ? int len, size, ret;
>
> ? ? ? ?/* we're temporarily limited to two virtqueues per rvdev */
> ? ? ? ?if (id >= ARRAY_SIZE(rvdev->vring))
> ? ? ? ? ? ? ? ?return ERR_PTR(-EINVAL);
>
> - ? ? ? rvring = &rvdev->vring[id];
> + ? ? ? ret = rproc_alloc_vring(rvdev, id);
> + ? ? ? if (ret)
> + ? ? ? ? ? ? ? return ERR_PTR(ret);
>
> + ? ? ? rvring = &rvdev->vring[id];
> ? ? ? ?addr = rvring->va;
> ? ? ? ?len = rvring->len;
>
> @@ -103,6 +106,7 @@ static struct virtqueue *rp_find_vq(struct
> virtio_device *vdev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rproc_virtio_notify, callback,
> name);
> ? ? ? ?if (!vq) {
> ? ? ? ? ? ? ? ?dev_err(rproc->dev, "vring_new_virtqueue %s failed\n",
> name);
> + ? ? ? ? ? ? ? rproc_free_vring(rvring);
> ? ? ? ? ? ? ? ?return ERR_PTR(-ENOMEM);
> ? ? ? ?}
>
> @@ -125,6 +129,7 @@ static void rproc_virtio_del_vqs(struct virtio_device
> *vdev)
> ? ? ? ? ? ? ? ?rvring = vq->priv;
> ? ? ? ? ? ? ? ?rvring->vq = NULL;
> ? ? ? ? ? ? ? ?vring_del_virtqueue(vq);
> + ? ? ? ? ? ? ? rproc_free_vring(rvring);
> ? ? ? ?}
> ?}
>
> @@ -228,8 +233,12 @@ static struct virtio_config_ops
> rproc_virtio_config_ops = {
> ?static void rproc_vdev_release(struct device *dev)
> ?{
> ? ? ? ?struct virtio_device *vdev = dev_to_virtio(dev);
> + ? ? ? struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> ? ? ? ?struct rproc *rproc = vdev_to_rproc(vdev);
>
> + ? ? ? list_del(&rvdev->node);
> + ? ? ? kfree(rvdev);
> +
> ? ? ? ?kref_put(&rproc->refcount, rproc_release);
> ?}
>
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/