Re: [lng-odp] [PATCH v1] Netmap pktio fixes
muvarov replied on github web page:
platform/linux-generic/pktio/netmap.c
line 14
@@ -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)
Comment:
@matiaselo @lumag yep, I forgot that some syscalls might be not implemented on
some systems, that also will return error.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @matiaselo yes, so this is fine.
>> Matias Elo(matiaselo) wrote:
>> 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.
>>> muvarov wrote
>>> .
muvarov wrote
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.
> muvarov wrote
> @lumag we also use SYS_gettid() in shm and timer. I think that is not
> subject for this PR. Just general clean up.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> See
>> [here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
>> for an interesting discussion of why gettid() is not used.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> @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.
muvarov wrote
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...
> Matias Elo(matiaselo) wrote:
> What's the benefit from using gettid()? It seems like the only
> difference is that gettid() cannot fail.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Why don't you use `gettid()` function? Then you can check for its
>> existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r147060031
updated_at 2017-10-26 07:09:00
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 14
@@ -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)
Comment:
@matiaselo yes, so this is fine.
> Matias Elo(matiaselo) wrote:
> 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.
>> muvarov wrote
>> .
>>> muvarov wrote
>>> 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.
muvarov wrote
@lumag we also use SYS_gettid() in shm and timer. I think that is not
subject for this PR. Just general clean up.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> See
> [here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
> for an interesting discussion of why gettid() is not used.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> @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.
>>> muvarov wrote
>>> 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...
Matias Elo(matiaselo) wrote:
What's the benefit from using gettid()? It seems like the only
difference is that gettid() cannot fail.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Why don't you use `gettid()` function? Then you can check for its
> existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r146858117
updated_at 2017-10-25 13:36:15
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 14
@@ -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)
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.
> muvarov wrote
> .
>> muvarov wrote
>> 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.
>>> muvarov wrote
>>> @lumag we also use SYS_gettid() in shm and timer. I think that is not
>>> subject for this PR. Just general clean up.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
See
[here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
for an interesting discussion of why gettid() is not used.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @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.
>> muvarov wrote
>> 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...
>>> Matias Elo(matiaselo) wrote:
>>> What's the benefit from using gettid()? It seems like the only
>>> difference is that gettid() cannot fail.
Dmitry Eremin-Solenikov(lumag) wrote:
Why don't you use `gettid()` function? Then you can check for its
existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r146774223
updated_at 2017-10-25 07:32:34
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
muvarov replied on github web page:
platform/linux-generic/pktio/netmap.c
line 14
@@ -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)
Comment:
.
> muvarov wrote
> 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.
>> muvarov wrote
>> @lumag we also use SYS_gettid() in shm and timer. I think that is not
>> subject for this PR. Just general clean up.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> See
>>> [here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
>>> for an interesting discussion of why gettid() is not used.
Dmitry Eremin-Solenikov(lumag) wrote:
@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.
> muvarov wrote
> 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...
>> Matias Elo(matiaselo) wrote:
>> What's the benefit from using gettid()? It seems like the only
>> difference is that gettid() cannot fail.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Why don't you use `gettid()` function? Then you can check for its
>>> existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r146576250
updated_at 2017-10-24 14:22:49
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
muvarov replied on github web page:
platform/linux-generic/pktio/netmap.c
line 14
@@ -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)
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.
> muvarov wrote
> @lumag we also use SYS_gettid() in shm and timer. I think that is not
> subject for this PR. Just general clean up.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> See
>> [here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
>> for an interesting discussion of why gettid() is not used.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> @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.
muvarov wrote
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...
> Matias Elo(matiaselo) wrote:
> What's the benefit from using gettid()? It seems like the only difference
> is that gettid() cannot fail.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Why don't you use `gettid()` function? Then you can check for its
>> existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145746832
updated_at 2017-10-19 16:04:32
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
muvarov replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
Comment:
@lumag we also use SYS_gettid() in shm and timer. I think that is not subject
for this PR. Just general clean up.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> See
> [here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
> for an interesting discussion of why gettid() is not used.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> @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.
>>> muvarov wrote
>>> 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...
Matias Elo(matiaselo) wrote:
What's the benefit from using gettid()? It seems like the only difference
is that gettid() cannot fail.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Why don't you use `gettid()` function? Then you can check for its
> existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145740741
updated_at 2017-10-19 15:42:59
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
Comment:
See
[here](https://stackoverflow.com/questions/21279649/getting-error-in-c-program-undefined-reference-to-gettid)
for an interesting discussion of why gettid() is not used.
> Dmitry Eremin-Solenikov(lumag) wrote:
> @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.
>> muvarov wrote
>> 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...
>>> Matias Elo(matiaselo) wrote:
>>> What's the benefit from using gettid()? It seems like the only difference
>>> is that gettid() cannot fail.
Dmitry Eremin-Solenikov(lumag) wrote:
Why don't you use `gettid()` function? Then you can check for its
existence in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145561678
updated_at 2017-10-18 22:38:19
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
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.
> muvarov wrote
> 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...
>> Matias Elo(matiaselo) wrote:
>> What's the benefit from using gettid()? It seems like the only difference is
>> that gettid() cannot fail.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Why don't you use `gettid()` function? Then you can check for its existence
>>> in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145384975
updated_at 2017-10-18 11:23:59
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
muvarov replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
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...
> Matias Elo(matiaselo) wrote:
> What's the benefit from using gettid()? It seems like the only difference is
> that gettid() cannot fail.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Why don't you use `gettid()` function? Then you can check for its existence
>> in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145377591
updated_at 2017-10-18 10:44:43
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
Comment:
What's the benefit from using gettid()? It seems like the only difference is
that gettid() cannot fail.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Why don't you use `gettid()` function? Then you can check for its existence
> in `configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145375871
updated_at 2017-10-18 10:36:13
Re: [lng-odp] [PATCH v1] Netmap pktio fixes
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/pktio/netmap.c
line 12
@@ -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);
Comment:
Why don't you use `gettid()` function? Then you can check for its existence in
`configure.ac` and provide replacement implementation.
https://github.com/Linaro/odp/pull/237#discussion_r145339479
updated_at 2017-10-18 07:54:51
