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.

Sign up for GitHub

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

Jump to bottom

[PATCH v1] Netmap pktio fixes #237

Closed
wants to merge 2 commits into from
Closed

[PATCH v1] Netmap pktio fixes #237

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4542584
linux-gen: netmap: use thread id to make vdev mac addresses unique
MatiasElo Mar 16, 2017
aa79b91
linux-gen: netmap: remove minimum frame len check
MatiasElo Mar 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 10 additions & 5 deletions platform/linux-generic/pktio/netmap.c
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
#include <protocols/eth.h>

#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <poll.h>
#include <linux/ethtool.h>
#include <linux/sockios.h>
Expand Down Expand 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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use gettid() function? Then you can check for its existence in configure.ac and provide replacement implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit from using gettid()? It seems like the only difference is that gettid() cannot fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for an interesting discussion of why gettid() is not used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lumag we also use SYS_gettid() in shm and timer. I think that is not subject for this PR. Just general clean up.


if ((int)tid == -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatiasElo yes, so this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatiasElo @lumag yep, I forgot that some syscalls might be not implemented on some systems, that also will return error.

ODP_DBG("Unable to fetch thread ID. VALE port MAC "
"addresses may not be unique.\n");

pkt_nm->capa.max_input_queues = 1;
pkt_nm->capa.set_op.op.promisc_mode = 0;
pkt_nm->mtu = buf_size;
pktio_entry->s.stats_type = STATS_UNSUPPORTED;
/* Set MAC address for virtual interface */
pkt_nm->if_mac[0] = 0x2;
pkt_nm->if_mac[1] = (tid >> 24) & 0xff;
pkt_nm->if_mac[2] = (tid >> 16) & 0xff;
pkt_nm->if_mac[3] = (tid >> 8) & 0xff;
pkt_nm->if_mac[4] = tid & 0xff;
pkt_nm->if_mac[5] = ++mac;

return 0;
Expand Down Expand Up @@ -639,11 +649,6 @@ static inline int netmap_pkt_to_odp(pktio_entry_t *pktio_entry,
goto fail;
}

if (odp_unlikely(len < _ODP_ETH_LEN_MIN)) {
ODP_ERR("RX: Frame truncated: %" PRIu16 "\n", len);
goto fail;
}

if (pktio_cls_enabled(pktio_entry)) {
if (cls_classify_packet(pktio_entry,
(const uint8_t *)slot.buf, len,
Expand Down