DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: David Marchand <david.marchand@6wind.com>,
	dev@dpdk.org, sergio.gonzalez.monroy@intel.com,
	ferruh.yigit@intel.com, kevin.traynor@intel.com,
	pmatilai@redhat.com
Subject: Re: [dpdk-dev] [PATCH] dropping librte_ivshmem - was log: deprecate history dump
Date: Thu, 09 Jun 2016 23:26:23 +0200	[thread overview]
Message-ID: <1679257.PTMOF8o7eO@xps13> (raw)
In-Reply-To: <1799099.25AIKSsmQj@xps13>

Looking a bit more into librte_ivshmem, the documentation says we need
a Qemu patch but the URL doesn't exist anymore:
	https://01.org/packet-processing/intel%C2%AE-ovdk
	-> 404 Oops, we couldn't find that page

I've never understood why we should keep this wart and now I'm going
to be upset.
To sum up the situation, eal depends on ivshmem which depends on
ring/mempool which depends... on eal.
The truth is that eal should not depends on librte_ivshmem.
And the option CONFIG_RTE_LIBRTE_IVSHMEM should not exist.

There are 3 parts to distinguish:

1/ The librte_ivshmem API to export some data structures from host.
No real problem here.

2/ The scan of the ivshmem devices in the guest init.
It should be handled as any other PCI device with an appropriate driver.
The scan is done by rte_eal_pci_init.

3/ The automatic mapped allocation of DPDK objects in the guest.
It should not be done in EAL.
An ivshmem driver would be called by rte_eal_dev_init.
It would check where are the shared DPDK structures, as currently done
with the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
Thus only the driver would depend on ring and mempool.

The last step of the ivshmem cleanup will be to remove the memory hack
RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM could be
removed.

So this is my proposal:
Someone start working on the above cleanup now, otherwise the whole
rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
We already talked about the rte_ivshmem design issues several times
and nobody declared using it.


---- original thread for reference ----

2016-06-09 17:01, Thomas Monjalon:
> 2016-06-09 16:45, David Marchand:
> > - Since you are looking at this, what keeps us from removing the
> > dependency on librte_ring ?
> 
> Please see this first small cleanup:
> 	http://dpdk.org/ml/archives/dev/2016-June/040798.html
> 
> > I would say it was mainly because of mempool.
> > Maybe ivshmem ?
> 
> Yes CONFIG_RTE_LIBRTE_IVSHMEM brings dependencies to rte_ring and rte_ivshmem.
> This "feature" also pollute the memory allocator and makes rework harder.
> That's why I would be in favor of removing CONFIG_RTE_LIBRTE_IVSHMEM.
> 
> Otherwise, as an alternative proposal, the file
> 	lib/librte_eal/linuxapp/eal/eal_ivshmem.c
> could be moved outside of EAL. Probably that lib/librte_ivshmem/
> would be a good place.
> The tricky operation would be to remove ivshmem init from eal:
> 
> #ifdef RTE_LIBRTE_IVSHMEM
>     if (rte_eal_ivshmem_init() < 0)                                                                              
>         rte_panic("Cannot init IVSHMEM\n");
> #endif
> 
>     if (rte_eal_memory_init() < 0)
>         rte_panic("Cannot init memory\n");
> 
>     /* the directories are locked during eal_hugepage_info_init */
>     eal_hugedirs_unlock();
> 
>     if (rte_eal_memzone_init() < 0)
>         rte_panic("Cannot init memzone\n");
> 
>     if (rte_eal_tailqs_init() < 0)
>         rte_panic("Cannot init tail queues for objects\n");
> 
> #ifdef RTE_LIBRTE_IVSHMEM
>     if (rte_eal_ivshmem_obj_init() < 0)
>         rte_panic("Cannot init IVSHMEM objects\n");
> #endif

  reply	other threads:[~2016-06-09 21:26 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-09 14:09 [dpdk-dev] [PATCH] " Thomas Monjalon
2016-06-09 14:45 ` David Marchand
2016-06-09 15:01   ` Thomas Monjalon
2016-06-09 21:26     ` Thomas Monjalon [this message]
2016-06-10  9:05       ` [dpdk-dev] [PATCH] dropping librte_ivshmem - was " Burakov, Anatoly
2016-06-10  9:30         ` Thomas Monjalon
2016-06-10  9:47           ` Burakov, Anatoly
2016-06-10 10:13             ` Thomas Monjalon
2016-06-10 12:08               ` Burakov, Anatoly
2016-06-10 12:26                 ` Thomas Monjalon
2016-06-15 18:16       ` Ferruh Yigit
2016-06-15 18:34         ` [dpdk-dev] [PATCH] dropping librte_ivshmem Thomas Monjalon
2016-06-20 15:44           ` Ferruh Yigit
2016-06-20 15:54             ` Thomas Monjalon
2016-06-21  6:49       ` [dpdk-dev] [PATCH] dropping librte_ivshmem - was log: deprecate history dump Panu Matilainen
2016-06-09 15:01   ` [dpdk-dev] [PATCH] " Christian Ehrhardt
2016-06-09 15:06 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2016-06-09 22:10   ` [dpdk-dev] [PATCH v3] " Thomas Monjalon
2016-06-10  9:50     ` David Marchand
2016-06-10 13:09       ` Thomas Monjalon

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=1679257.PTMOF8o7eO@xps13 \
    --to=thomas.monjalon@6wind.com \
    --cc=anatoly.burakov@intel.com \
    --cc=david.marchand@6wind.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=kevin.traynor@intel.com \
    --cc=pmatilai@redhat.com \
    --cc=sergio.gonzalez.monroy@intel.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).