msg/async/dpdk: ms_dpdk_coremask can skip the first few cores #20638
Conversation
src/msg/async/dpdk/dpdk_rte.cc
Outdated
| @@ -45,6 +45,29 @@ namespace dpdk { | |||
| return std::bitset<CHAR_BIT * sizeof(n)>{n}.count(); | |||
| } | |||
|
|
|||
| uint16_t eal::get_first_slave_coreid(const char *coremask) | |||
| { | |||
| uint64_t ulCoreMask; | |||
There was a problem hiding this comment.
might want to avoid using camelCase and Hungarian naming convention, instead could you use something like core_mask ?
There was a problem hiding this comment.
thx, i have modify code style as you said, pls check again.
src/msg/async/dpdk/dpdk_rte.cc
Outdated
| int iRet = 0; | ||
|
|
||
| iRet = sscanf(coremask, "%lx", &ulCoreMask); | ||
| if (1 != iRet || 0 == ulCoreMask) |
There was a problem hiding this comment.
nit, i'd suggest avoid using yoda conditions. as modern compiler will print warnings like
warning: suggest parentheses around assignment used as truth value [-Wparentheses]
at seeing
if (foo = 0) {
heal(the_world);
}There was a problem hiding this comment.
now it's this:
auto core_value = stoull(coremask_arg, nullptr, 16);
and ,thers is no warning.
src/msg/async/dpdk/dpdk_rte.cc
Outdated
| return 0; | ||
| } | ||
|
|
||
| for (usBitCount = 0; 0 == ulCoreMask % 2 && 0 != ulCoreMask; usBitCount++) |
There was a problem hiding this comment.
no need to reinvent the wheel, could just use ffsll() for the index of the MSB.
src/msg/async/dpdk/dpdk_rte.cc
Outdated
| uint16_t first_slave_coreid; | ||
| int iRet = 0; | ||
|
|
||
| iRet = sscanf(coremask, "%lx", &ulCoreMask); |
There was a problem hiding this comment.
might want to use std::stoull() instead. simpler this way.
|
@liu-chunmei in case you are interested. |
Signed-off-by: shangfufei <shangfufei@inspur.com>
shangfufei
force-pushed
the
dpdk-coremask
branch
from
b561123
to
f56e773
Compare
There was a problem hiding this comment.
@shangfufei after a second thought, i think we can have a simpler fix for this, see #20659. could you take a look?
| ceph_abort(); | ||
| } | ||
|
|
||
| if (first_slave_coreid == 1) |
There was a problem hiding this comment.
move this check into the try block, and throw std::invalid_argument("core mask specifies no cores")
| auto core_value = stoull(coremask_arg, nullptr, 16); | ||
| first_slave_coreid = ffsll(core_value) + 1; | ||
| } catch (const std::logic_error& e) { | ||
| lderr(cct) << __func__ << " invalid ms_spdk_coremask: " |
There was a problem hiding this comment.
ok, it's should be:
lderr(cct) << func << " invalid ms_dpdk_coremask: "
| @@ -250,8 +250,26 @@ void DPDKStack::spawn_worker(unsigned i, std::function<void ()> &&func) | |||
| // if dpdk::eal::init already called by NVMEDevice, we will select 1..n | |||
| // cores | |||
| assert(rte_lcore_count() >= i + 1); | |||
| auto coremask_arg = cct->_conf->get_val<std::string>("ms_dpdk_coremask"); | |||
| int first_slave_coreid = -1; | |||
There was a problem hiding this comment.
s/first_slave_coreid/first_core_id/, as the slave does not match any of concepts, terms or variable names in async messenger.
There was a problem hiding this comment.
more than one cores are usually used in the process based on dpdk. one works as master core, and others works as slave core. so i take the id of first slave core as first_slave_coreid.
| << at least one core is needed << dendl; | ||
| ceph_abort(); | ||
| } | ||
| ldout(cct, 5) << __func__ << " coremask:" << cct->_conf->ms_dpdk_coremask << " first_slave_coreid = " << first_slave_coreid << dendl; |
There was a problem hiding this comment.
no point to print this message. it's merely a translation of the option specified by user. and it is printed multiple times for each worker.
There was a problem hiding this comment.
well, you are right, del this debug log infor.
|
see #20659 |
| if (first_slave_coreid == 1) | ||
| { | ||
| lderr(cct) << __func__ << " invalid ms_spdk_coremask: " | ||
| << at least one core is needed << dendl; |
There was a problem hiding this comment.
thx, this log infor can be changed just like this:
if (first_slave_coreid == 1)
{
lderr(cct) << func << " invalid ms_dpdk_coremask: "
<< at least one core is needed << dendl;
There was a problem hiding this comment.
we should allocate cores for each process of ceph， when running with dpdk. So we have to make clear the core of each process. If a process has only a primary core and no auxiliary core, then this process can not start success. Based on this consideration, I added this logic .
|
closing in favor of #20659 |
ms_dpdk_coremask can skip the first few cores, such as when ms_dpdk_coremask = 0xF0, the master core id is 4, and the fist slave core id is 5.
Signed-off-by: shangfufei shangfufei@inspur.com