Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract DPDK dependences with SPDK env APIs - Take 2 #136

Closed
wants to merge 6 commits into from

Conversation

@johnmeneghini
Copy link
Contributor

johnmeneghini commented Apr 11, 2017

- Implemented a wrapper funtions for rte_config.h, rte_mempool.h, rte_lcore.h APIs.
- Updated lib/env_dpdk with new spdk APIs to call rte functions
- Changes to nvmf_tgt, event, reactor and app code to call the new
  spdk env wrapper functions instead of rte_ functions directly
  - update env.c and env.h with new spdk_ring apis
  - update reactor.c with new abstractions
  - added SPDK_MAX_LCORE to abstract RTE_MAX_LCORE
  - added addtional spdk_lcore routines to env.c
  - removed rte_config and rte_lcore dependencies from reactor.c, bdev.c and app.c
  - added SPDK_MEMPOOL_CACHE_MAX_SIZE to abstract RTE_MEMPOOL_CACHE_MAX_SIZE
  - added spdk_mempool_avail_count and moved rte version specific code into env layer
  - converted bdev.c to use spdk_mempool abstractions
  - small formatting changes in reactor.c
  - update copy_engine.c and remove rte_memcpy.h dependency
  - hack to work around environments that don't support rlimits
@@ -39,9 +39,6 @@
#include <string.h>
#include <unistd.h>

#include <rte_config.h>

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

I cherry-picked this commit as decea46, thanks!

#define RING_F_SC_DEQ 0x0002 /**< The default dequeue is "single-consumer". */

/**
* Enqueue several objects on the ring (multi-producers safe).

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

Many of these comments look identical to the ones from DPDK's headers; if so, we should also copy the corresponding copyright information.

@@ -126,6 +131,8 @@ void spdk_memzone_dump(FILE *f);

struct spdk_mempool;

#define SPDK_MEMPOOL_CACHE_MAX_SIZE 512

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

This value is configurable in DPDK; I don't think we can hard-code it like this in env.h. I believe this value is only used in initialization paths that aren't performance critical, so we could add an accessor function to get this value rather than a #define.

This comment has been minimized.

Copy link
@johnmeneghini

johnmeneghini Apr 25, 2017

Author Contributor

Jim Harris and I discussed this. The plan is to include a compile time assert in env.c that insures this definition, as well as a couple of others, don't get out of sync with what's in DPDK.

* - -ENOBUFS: Not enough room in the ring to enqueue, no object is enqueued.
*/
int
spdk_ring_mp_enqueue_bulk(struct spdk_ring *sr, void *const *obj_table,

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

Function declarations should not split the line after the return type (only function definitions should do that).

There are some places where we aren't consistent on this, but we should fix those and avoid adding new ones.

This comment has been minimized.

Copy link
@johnmeneghini

johnmeneghini Apr 25, 2017

Author Contributor

OK. So we want all declarations on a single line.

*/
struct spdk_ring *spdk_ring_lookup(const char *name);

#define SPDK_MAX_LCORE 128

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

This is another DPDK configuration setting, so I don't think we want to hard-code 128 here. It's already possible in extreme cases to have more than 128 logical cores on a single system (e.g. 8-socket systems).

This is a little trickier to avoid hard-coding, since we do want to use it to size static arrays; possibly we can replace those static arrays with dynamic allocations and turn this into an accessor function as well.

This comment has been minimized.

Copy link
@johnmeneghini

johnmeneghini Apr 25, 2017

Author Contributor

This there is a compile time ASSERT to take care of this.

* Memcpy
*/
void
spdk_memcpy(void *dst, const void *src, int len);

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

I don't think we really need to expose our own memcpy if we aren't going to inline it. The main benefit of rte_memcpy() is the specialization for inlined versions where the size is known at compile time.

This comment has been minimized.

Copy link
@johnmeneghini

johnmeneghini Apr 25, 2017

Author Contributor

We need to abstract rte_memcpy(). If this function doesn't work because is't not inlined, we can work on that. But using rte_memcpy doesn't work for us.

This comment has been minimized.

Copy link
@ranjitnoronha

ranjitnoronha Apr 26, 2017

I agree. We also need our own version of memcpy(). Calling rte_memcpy() irrespective of its internal optimizations does not work for us.

* Insure the definitions in spdk/env.h are equivelant to the DPDK definitions.
*/

static_assert(SPDK_MAX_LCORE == RTE_MAX_LCORE, "SPDK_MAX_LCORE != RTE_MAX_LCORE");

This comment has been minimized.

Copy link
@ghost

ghost Apr 25, 2017

These should use SPDK_STATIC_ASSERT() from spdk/assert.h for compatibility with old compilers that don't have static_assert.

This comment has been minimized.

Copy link
@johnmeneghini

johnmeneghini Apr 25, 2017

Author Contributor

OK. I can fix this.

@johnmeneghini

This comment has been minimized.

Copy link
Contributor Author

johnmeneghini commented Apr 26, 2017

@johnmeneghini

This comment has been minimized.

Copy link
Contributor Author

johnmeneghini commented May 3, 2017

This pull request has been replaced by #152
I'm closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.