DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Elad Nachman <eladv6@gmail.com>
Cc: <dev@dpdk.org>, <erclists@gmail.com>, <iryzhov@nfware.com>
Subject: Re: [dpdk-dev] [PATCH] kni: Fix request overwritten
Date: Tue, 21 Sep 2021 14:57:46 +0100	[thread overview]
Message-ID: <8b0ea7c1-9f2f-c081-ff01-6d327eaafc1c@intel.com> (raw)
In-Reply-To: <20210917123131.6329-1-eladv6@gmail.com>

On 9/17/2021 1:31 PM, Elad Nachman wrote:
> Fix lack of multiple KNI requests handling support by introducing a 
> rotating ring mechanism for the sync buffer.
> 

Thanks Elad for the patch.

I have missed that 'rte_kni_handle_request()' can be called by multiple cores,
at least kni sample app does that.

In that case having multiple requests may end up multiple control command run at
the same time from different cores and this may lead undefined behavior.

Forcing to have single request at a time also simplifies to seriliaze the
control commands to the device.

What about your suggestion to return -EAGAIN, if the request is not processed by
the userspace. And this can be detected by a new field in 'rte_kni_request'
which kernel sets when the request issued and userspace unset when the
processing done. 'kni_net_process_request()' can require that field to be unset
before putting a new request. What do you think?

> Prevent kni_net_change_rx_flags() from calling kni_net_process_request()
> with a malformed request.
> 

This is completely something else so can be dropped from this patch, but OK to
add 'if (req.req_id)' check although current request is harmless since 'req' is
already memset.

> Bugzilla ID: 809
> 
> Signed-off-by: Elad Nachman <eladv6@gmail.com>
> ---
>  kernel/linux/kni/kni_dev.h  |  2 ++
>  kernel/linux/kni/kni_misc.c |  2 ++
>  kernel/linux/kni/kni_net.c  | 25 +++++++++++++++++--------
>  lib/kni/rte_kni.c           | 14 +++++++-------
>  lib/kni/rte_kni_common.h    |  1 +
>  5 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
> index c15da311ba..b9e8b3d10d 100644
> --- a/kernel/linux/kni/kni_dev.h
> +++ b/kernel/linux/kni/kni_dev.h
> @@ -74,6 +74,8 @@ struct kni_dev {
>  
>  	void *sync_kva;
>  	void *sync_va;
> +	unsigned int sync_ring_size;
> +	atomic_t sync_ring_idx;
>  
>  	void *mbuf_kva;
>  	void *mbuf_va;
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 2b464c4381..f3cee97818 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -345,6 +345,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>  
>  	kni->net_dev = net_dev;
>  	kni->core_id = dev_info.core_id;
> +	kni->sync_ring_size = dev_info.sync_ring_size;
> +	kni->sync_ring_idx.counter = 0;
>  	strncpy(kni->name, dev_info.name, RTE_KNI_NAMESIZE);
>  
>  	/* Translate user space info into kernel space info */
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 611719b5ee..dc066cdd98 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -110,6 +110,8 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  	void *resp_va;
>  	uint32_t num;
>  	int ret_val;
> +	unsigned int idx;
> +	char *k_reqptr, *v_reqptr;
>  
>  	ASSERT_RTNL();
>  
> @@ -122,10 +124,17 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  	}
>  
>  	mutex_lock(&kni->sync_lock);
> -
> +	idx = atomic_read(&kni->sync_ring_idx);
> +	atomic_cmpxchg(&kni->sync_ring_idx, idx,
> +		(idx + 1) % kni->sync_ring_size);
>  	/* Construct data */
> -	memcpy(kni->sync_kva, req, sizeof(struct rte_kni_request));
> -	num = kni_fifo_put(kni->req_q, &kni->sync_va, 1);
> +	k_reqptr = (char *)((uintptr_t)kni->sync_kva +
> +			sizeof(struct rte_kni_request) * idx);
> +	v_reqptr = (char *)((uintptr_t)kni->sync_va +
> +			sizeof(struct rte_kni_request) * idx);
> +
> +	memcpy(k_reqptr, req, sizeof(struct rte_kni_request));
> +	num = kni_fifo_put(kni->req_q, (void **)&v_reqptr, 1);
>  	if (num < 1) {
>  		pr_err("Cannot send to req_q\n");
>  		ret = -EBUSY;
> @@ -147,14 +156,14 @@ kni_net_process_request(struct net_device *dev, struct rte_kni_request *req)
>  		goto fail;
>  	}
>  	num = kni_fifo_get(kni->resp_q, (void **)&resp_va, 1);
> -	if (num != 1 || resp_va != kni->sync_va) {
> +	if (num != 1) {
>  		/* This should never happen */
>  		pr_err("No data in resp_q\n");
>  		ret = -ENODATA;
>  		goto fail;
>  	}
>  
> -	memcpy(req, kni->sync_kva, sizeof(struct rte_kni_request));
> +	memcpy(req, k_reqptr, sizeof(struct rte_kni_request));
>  async:
>  	ret = 0;
>  
> @@ -681,13 +690,12 @@ kni_net_change_mtu(struct net_device *dev, int new_mtu)
>  static void
>  kni_net_change_rx_flags(struct net_device *netdev, int flags)
>  {
> -	struct rte_kni_request req;
> +	struct rte_kni_request req = { 0 };
>  
>  	memset(&req, 0, sizeof(req));
>  
>  	if (flags & IFF_ALLMULTI) {
>  		req.req_id = RTE_KNI_REQ_CHANGE_ALLMULTI;
> -
>  		if (netdev->flags & IFF_ALLMULTI)
>  			req.allmulti = 1;
>  		else
> @@ -703,7 +711,8 @@ kni_net_change_rx_flags(struct net_device *netdev, int flags)
>  			req.promiscusity = 0;
>  	}
>  
> -	kni_net_process_request(netdev, &req);
> +	if (req.req_id)
> +		kni_net_process_request(netdev, &req);
>  }
>  
>  /*
> diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c
> index d3e236005e..e921d41ce8 100644
> --- a/lib/kni/rte_kni.c
> +++ b/lib/kni/rte_kni.c
> @@ -31,6 +31,9 @@
>  #define KNI_FIFO_COUNT_MAX     1024
>  #define KNI_FIFO_SIZE          (KNI_FIFO_COUNT_MAX * sizeof(void *) + \
>  					sizeof(struct rte_kni_fifo))
> +#define KNI_REQS_SIZE          (KNI_FIFO_COUNT_MAX * \
> +					sizeof(struct rte_kni_request) + \
> +					sizeof(struct rte_kni_fifo))
>  
>  #define KNI_REQUEST_MBUF_NUM_MAX      32
>  
> @@ -175,8 +178,8 @@ kni_reserve_mz(struct rte_kni *kni)
>  	KNI_MEM_CHECK(kni->m_resp_q == NULL, resp_q_fail);
>  
>  	snprintf(mz_name, RTE_MEMZONE_NAMESIZE, KNI_SYNC_ADDR_MZ_NAME_FMT, kni->name);
> -	kni->m_sync_addr = rte_memzone_reserve(mz_name, KNI_FIFO_SIZE, SOCKET_ID_ANY,
> -			RTE_MEMZONE_IOVA_CONTIG);
> +	kni->m_sync_addr = rte_memzone_reserve(mz_name, KNI_REQS_SIZE,
> +			SOCKET_ID_ANY, RTE_MEMZONE_IOVA_CONTIG);
>  	KNI_MEM_CHECK(kni->m_sync_addr == NULL, sync_addr_fail);
>  
>  	return 0;
> @@ -307,6 +310,7 @@ rte_kni_alloc(struct rte_mempool *pktmbuf_pool,
>  	kni->sync_addr = kni->m_sync_addr->addr;
>  	dev_info.sync_va = kni->m_sync_addr->addr;
>  	dev_info.sync_phys = kni->m_sync_addr->iova;
> +	dev_info.sync_ring_size = KNI_FIFO_COUNT_MAX;
>  
>  	kni->pktmbuf_pool = pktmbuf_pool;
>  	kni->group_id = conf->group_id;
> @@ -544,11 +548,7 @@ rte_kni_handle_request(struct rte_kni *kni)
>  	if (ret != 1)
>  		return 0; /* It is OK of can not getting the request mbuf */
>  
> -	if (req != kni->sync_addr) {
> -		RTE_LOG(ERR, KNI, "Wrong req pointer %p\n", req);
> -		return -1;
> -	}
> -
> +	printf("%s: req %p id %u\n", __func__, req, req->req_id);
>  	/* Analyze the request and call the relevant actions for it */
>  	switch (req->req_id) {
>  	case RTE_KNI_REQ_CHANGE_MTU: /* Change MTU */
> diff --git a/lib/kni/rte_kni_common.h b/lib/kni/rte_kni_common.h
> index b547ea5501..a78b9bafa0 100644
> --- a/lib/kni/rte_kni_common.h
> +++ b/lib/kni/rte_kni_common.h
> @@ -110,6 +110,7 @@ struct rte_kni_device_info {
>  	phys_addr_t resp_phys;
>  	phys_addr_t sync_phys;
>  	void * sync_va;
> +	unsigned int sync_ring_size;
>  
>  	/* mbuf mempool */
>  	void * mbuf_va;
> 


      reply	other threads:[~2021-09-21 14:15 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-17 12:31 Elad Nachman
2021-09-21 13:57 ` Ferruh Yigit [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8b0ea7c1-9f2f-c081-ff01-6d327eaafc1c@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=eladv6@gmail.com \
    --cc=erclists@gmail.com \
    --cc=iryzhov@nfware.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).