[PATCH v1] Netmap pktio fixes #237
Conversation
Previously the mac addresses of virtual netmap devices would clash if multiple odp processes were run on the same host. Signed-off-by: Matias Elo <matias.elo@nokia.com>
Checking the minimum frame length is unnecessary as netmap drops truncated frames internally. Signed-off-by: Matias Elo <matias.elo@nokia.com>
| @@ -388,13 +389,22 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, pktio_entry_t *pktio_entry, | |||
|
|
|||
| if (pkt_nm->is_virtual) { | |||
| static unsigned mac; | |||
| uint32_t tid = syscall(SYS_gettid); | |||
There was a problem hiding this comment.
Why don't you use gettid() function? Then you can check for its existence in configure.ac and provide replacement implementation.
There was a problem hiding this comment.
What's the benefit from using gettid()? It seems like the only difference is that gettid() cannot fail.
There was a problem hiding this comment.
it was copied from netmap source I think. gittid() will generate warning due to missing glibc wrapper: https://stackoverflow.com/questions/30680550/c-gettid-was-not-declared-in-this-scope Maybe something already changed...
There was a problem hiding this comment.
@MatiasElo @muvarov Hmm. I thought that there is already a Glibc wrapper. I would prefer this as a separate function, but it is of minor priority then.
There was a problem hiding this comment.
See here for an interesting discussion of why gettid() is not used.
There was a problem hiding this comment.
@lumag we also use SYS_gettid() in shm and timer. I think that is not subject for this PR. Just general clean up.
| @@ -388,13 +389,22 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, pktio_entry_t *pktio_entry, | |||
|
|
|||
| if (pkt_nm->is_virtual) { | |||
| static unsigned mac; | |||
| uint32_t tid = syscall(SYS_gettid); | |||
There was a problem hiding this comment.
See here for an interesting discussion of why gettid() is not used.
| @@ -388,13 +389,22 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, pktio_entry_t *pktio_entry, | |||
|
|
|||
| if (pkt_nm->is_virtual) { | |||
| static unsigned mac; | |||
| uint32_t tid = syscall(SYS_gettid); | |||
|
|
|||
| if ((int)tid == -1) | |||
There was a problem hiding this comment.
man gettid says that it always passes.
kernel code is also:
pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
struct pid_namespace *ns)
{
pid_t nr = 0;
rcu_read_lock();
if (!ns)
ns = task_active_pid_ns(current);
if (likely(pid_alive(task))) {
if (type != PIDTYPE_PID)
task = task->group_leader;
nr = pid_nr_ns(rcu_dereference(task->pids[type].pid), ns);
}
rcu_read_unlock();
return nr;
}
EXPORT_SYMBOL(__task_pid_nr_ns);
I.e. it will return init process tid 0 in worst case. So this check is not correct and not needed.
It might be needed additional cast:
uint32_t tid = (uint32_t)syscall(SYS_gettid) because of syscall returns pid_t.
There was a problem hiding this comment.
syscall() specification still defines -1 as an error return value and we should adhere to the spec regardless of the function implementation which may change.
There was a problem hiding this comment.
@MatiasElo @lumag yep, I forgot that some syscalls might be not implemented on some systems, that also will return error.
|
Merged. |
No description provided.