Date:   Fri, 3 Feb 2023 15:07:04 +0530
From:   Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
To:     Elliot Berman <quic_eberman@quicinc.com>
CC:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Alex Elder <elder@linaro.org>,
        Murali Nalajala <quic_mnalajal@quicinc.com>,
        "Jonathan Corbet" <corbet@lwn.net>,
        Trilok Soni <quic_tsoni@quicinc.com>,
        "Carl van Schaik" <quic_cvanscha@quicinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "Will Deacon" <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v9 21/27] gunyah: vm_mgr: Add framework to add VM
 Functions
Message-ID: <20230203093704.GC332@quicinc.com>
Reply-To: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
References: <20230120224627.4053418-1-quic_eberman@quicinc.com>
 <20230120224627.4053418-22-quic_eberman@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
In-Reply-To: <20230120224627.4053418-22-quic_eberman@quicinc.com>
Precedence: bulk

* Elliot Berman <quic_eberman@quicinc.com> [2023-01-20 14:46:20]:

> +static struct gunyah_vm_function_driver *__find_function(const char name[GUNYAH_FUNCTION_NAME_SIZE])
> +	__must_hold(functions_lock)
> +{
> +	struct gunyah_vm_function_driver *iter, *drv = NULL;
> +
> +	list_for_each_entry(iter, &functions, list) {
> +		if (!strncmp(iter->name, name, GUNYAH_FUNCTION_NAME_SIZE)) {
> +			drv = iter;
> +			break;
> +		}
> +	}

Not sure how much of a hot path this is going to sit in. I can imagine VM boot
to be in fast path for some cases (VMs spawned on usecase boundaries - I think
some VMs like in Amazon firecracker boot in fraction of a second). This
indirection could cost that a bit (linear search + strcmp for the right
function). IMHO a direct interface (ex: ADD_IOEVENTFD) will be more efficient.

> +void gunyah_vm_function_unregister(struct gunyah_vm_function_driver *drv)
> +{
> +	struct gunyah_vm_function *f, *iter;
> +
> +	mutex_lock(&functions_lock);
> +	list_for_each_entry_safe(f, iter, &drv->instances, drv_list)
> +		gh_vm_remove_function(f);
> +	list_del(&drv->list);
> +	mutex_unlock(&functions_lock);

This seems to allow essential functions to be unregistered while there are still
active users. For example, it would allow ioeventfd or irqfd module to be
unloaded while there are VMs depending on it. I think it would be better if we
allow module unload (aka function_unregister) only after dependent VMs are stopped.

> +static long gh_vm_rm_function(struct gunyah_vm *ghvm, struct gh_vm_function *fn)
> +{
> +	long r = 0;
> +	struct gunyah_vm_function *f, *iter;
> +
> +	r = mutex_lock_interruptible(&functions_lock);
> +	if (r)
> +		return r;
> +
> +	list_for_each_entry_safe(f, iter, &ghvm->functions, vm_list) {
> +		if (!memcmp(&f->fn, fn, sizeof(*fn)))
> +			gh_vm_remove_function(f);
> +	}
> +

I think we should return some error (for ioctl atleast) if given function was
not found.

> +struct gh_vm_function {
> +	char name[GUNYAH_FUNCTION_NAME_SIZE];
> +	union {
> +		char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
> +	};

Can you find a way to optimize this memory/time usage? For example, in case of
ioevents we strictly need 28 bytes, but end up consuming 1kB.
Also we end up comparing 1024 bytes during REMOVE_FUNCTION when strictly 28
bytes or fewer bytes comparison was required in case of ioeventfd.