Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions GNUmakefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
BUILDDIR ?= build
BUILDTYPE ?= debugoptimized
SANITIZE ?= none
COVERAGE ?= false
V ?= 0
ifeq ($V,1)
ninja_opts = --verbose
Expand All @@ -19,6 +20,7 @@ all: $(BUILDDIR)/build.ninja
.PHONY: debug
debug: BUILDTYPE = debug
debug: SANITIZE = address
debug: COVERAGE = true
debug: all

.PHONY: unit-tests
Expand All @@ -42,8 +44,11 @@ update-graph: all
coverage:
$Q mkdir -p $(BUILDDIR)/coverage
$Q gcovr --html-details $(BUILDDIR)/coverage/index.html --txt \
-e '.*_test.c' -e 'subprojects/*' --gcov-ignore-parse-errors \
-ur . $(BUILDDIR)
-e '.*_test.c' -e 'subprojects/.*' --gcov-ignore-parse-errors \
--gcov-executable `$(CC) -print-prog-name=gcov` \
--object-directory $(BUILDDIR) \
--sort uncovered-percent \
-r . $(BUILDDIR)
@echo Coverage data is present in $(BUILDDIR)/coverage/index.html

.PHONY: all
Expand All @@ -55,7 +60,7 @@ install: $(BUILDDIR)/build.ninja
$Q meson install -C $(BUILDDIR) --skip-subprojects

meson_opts = --buildtype=$(BUILDTYPE) --werror --warnlevel=2
meson_opts += -Db_sanitize=$(SANITIZE) -Ddpdk_static=true
meson_opts += -Db_sanitize=$(SANITIZE) -Db_coverage=$(COVERAGE) -Ddpdk_static=true
meson_opts += $(MESON_EXTRA_OPTS)

$(BUILDDIR)/build.ninja:
Expand Down
10 changes: 2 additions & 8 deletions meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -132,19 +132,13 @@ install_headers(api_headers)
cmocka_dep = dependency('cmocka', required: get_option('tests'))
if cmocka_dep.found()
fs = import('fs')
coverage_c_args = []
coverage_link_args = []
if compiler.get_id() == 'gcc'
coverage_c_args += ['-coverage']
coverage_link_args += ['-lgcov']
endif
foreach t : tests
name = fs.replace_suffix(t['sources'].get(0), '').underscorify()
t += {
'sources': t['sources'] + files('api/string.c'),
'include_directories': inc + api_inc,
'c_args': ['-D__GROUT_MAIN__', '-D__GROUT_UNIT_TEST__'] + coverage_c_args,
'link_args': t['link_args'] + coverage_link_args,
'c_args': ['-D__GROUT_MAIN__', '-D__GROUT_UNIT_TEST__'],
'link_args': t['link_args'],
'dependencies': [dpdk_dep, ev_core_dep, ev_thread_dep, numa_dep, ecoli_dep, cmocka_dep],
}
test(name, executable(name, kwargs: t), suite: 'unit')
Expand Down
2 changes: 2 additions & 0 deletions modules/infra/api/gr_infra.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ typedef enum : uint16_t {
// Interface state flags
typedef enum : uint16_t {
GR_IFACE_S_RUNNING = GR_BIT16(0),
GR_IFACE_S_PROMISC_FIXED = GR_BIT16(1),
GR_IFACE_S_ALLMULTI_FIXED = GR_BIT16(2),
} gr_iface_state_t;

// Interface reconfig attributes
Expand Down
8 changes: 6 additions & 2 deletions modules/infra/cli/iface.c
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,13 @@ static ssize_t iface_flags_format(char *buf, size_t len, const struct gr_iface *
SAFE_BUF(snprintf, len, "down");
if (iface->state & GR_IFACE_S_RUNNING)
SAFE_BUF(snprintf, len, " running");
if (iface->flags & GR_IFACE_F_PROMISC)
if (iface->state & GR_IFACE_S_PROMISC_FIXED)
SAFE_BUF(snprintf, len, " promisc(fixed)");
else if (iface->flags & GR_IFACE_F_PROMISC)
SAFE_BUF(snprintf, len, " promisc");
if (iface->flags & GR_IFACE_F_ALLMULTI)
if (iface->state & GR_IFACE_S_ALLMULTI_FIXED)
SAFE_BUF(snprintf, len, " allmulti(fixed)");
else if (iface->flags & GR_IFACE_F_ALLMULTI)
SAFE_BUF(snprintf, len, " allmulti");
if (iface->flags & GR_IFACE_F_PACKET_TRACE)
SAFE_BUF(snprintf, len, " tracing");
Expand Down
16 changes: 15 additions & 1 deletion modules/infra/control/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,19 @@ tests += [
'-Wl,--wrap=rte_pktmbuf_pool_create',
'-Wl,--wrap=rte_zmalloc',
],
}
},
{
'sources': files('port_test.c', 'port.c'),
'link_args': [
'-Wl,--wrap=rte_eth_allmulticast_disable',
'-Wl,--wrap=rte_eth_allmulticast_enable',
'-Wl,--wrap=rte_eth_allmulticast_get',
'-Wl,--wrap=rte_eth_dev_mac_addr_add',
'-Wl,--wrap=rte_eth_dev_mac_addr_remove',
'-Wl,--wrap=rte_eth_dev_set_mc_addr_list',
'-Wl,--wrap=rte_eth_promiscuous_disable',
'-Wl,--wrap=rte_eth_promiscuous_enable',
'-Wl,--wrap=rte_eth_promiscuous_get',
],
},
]
71 changes: 59 additions & 12 deletions modules/infra/control/port.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: BSD-3-Clause
// Copyright (c) 2023 Robin Jarry

#include "port_priv.h"
#include "worker_priv.h"

#include <gr_config.h>
Expand Down Expand Up @@ -194,10 +195,13 @@ static int port_mac_set(struct iface *iface, const struct rte_ether_addr *mac) {
return 0;
}

static int port_promisc_set(struct iface *iface, bool enabled) {
int port_promisc_set(struct iface *iface, bool enabled) {
struct iface_info_port *p = iface_info_port(iface);
int ret;

if (iface->state & GR_IFACE_S_PROMISC_FIXED)
return 0; // promisc is forced to filter unicast addresses, leave it as-is

if (enabled)
ret = rte_eth_promiscuous_enable(p->port_id);
else
Expand All @@ -220,10 +224,13 @@ static int port_promisc_set(struct iface *iface, bool enabled) {
return 0;
}

static int port_allmulti_set(struct iface *iface, bool enabled) {
int port_allmulti_set(struct iface *iface, bool enabled) {
struct iface_info_port *p = iface_info_port(iface);
int ret;

if (iface->state & GR_IFACE_S_ALLMULTI_FIXED)
return 0; // allmulti is forced to filter multicast addresses, leave it as-is

if (enabled)
ret = rte_eth_allmulticast_enable(p->port_id);
else
Expand Down Expand Up @@ -496,7 +503,7 @@ static int port_mac_get(const struct iface *iface, struct rte_ether_addr *mac) {
return 0;
}

static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) {
int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) {
struct iface_info_port *port = iface_info_port(iface);
struct mac_filter *filter;
const char *mac_type;
Expand Down Expand Up @@ -566,19 +573,35 @@ static int port_mac_add(struct iface *iface, const struct rte_ether_addr *mac) {
else
ret = rte_eth_promiscuous_enable(port->port_id);

if (ret == 0)
if (ret == 0) {
filter->flags |= MAC_FILTER_F_ALL;
iface->state |= multicast ?
GR_IFACE_S_ALLMULTI_FIXED :
GR_IFACE_S_PROMISC_FIXED;
}
}

if (ret < 0) {
filter->count--;
return errno_log(-ret, mac_type);
}

if (filter->flags & MAC_FILTER_F_ALL) {
// Purge the device from all explicit addresses.
// We will add them back when removing the forced allmulti/promisc.
// Ignore the return values. They don't matter.
if (multicast) {
rte_eth_dev_set_mc_addr_list(port->port_id, filter->mac, 0);
} else {
for (i = 0; i < filter->hw_limit; i++)
rte_eth_dev_mac_addr_remove(port->port_id, &filter->mac[i]);
}
}

return 0;
}

static int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) {
int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) {
struct iface_info_port *port = iface_info_port(iface);
struct mac_filter *filter;
const char *mac_type;
Expand Down Expand Up @@ -648,15 +671,39 @@ static int port_mac_del(struct iface *iface, const struct rte_ether_addr *mac) {
iface->name,
multicast ? "allmulti" : "promisc",
rte_strerror(-ret));
}
else
iface->state &= multicast ?
~GR_IFACE_S_ALLMULTI_FIXED :
~GR_IFACE_S_PROMISC_FIXED;

// Reinstall all addresses after disabling allmulti/promisc.
if (multicast) {
ret = rte_eth_dev_set_mc_addr_list(
port->port_id, filter->mac, filter->count
);
} else {
for (i = 0; i < filter->count; i++) {
int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0);
if (r < 0 && ret == 0)
ret = r;
}
}
if (ret < 0)
LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret));
Comment on lines +674 to +692
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Update iface->flags when clearing forced mode.

After successfully disabling allmulti/promisc (lines 674-677), the code clears the state flags but doesn't update iface->flags to clear GR_IFACE_F_ALLMULTI/GR_IFACE_F_PROMISC. Since the hardware mode is being disabled, the flags should reflect that.

Apply this diff to clear the flags:

 		else
-			iface->state &= multicast ?
-				~GR_IFACE_S_ALLMULTI_FIXED :
-				~GR_IFACE_S_PROMISC_FIXED;
+			if (multicast) {
+				iface->state &= ~GR_IFACE_S_ALLMULTI_FIXED;
+				iface->flags &= ~GR_IFACE_F_ALLMULTI;
+			} else {
+				iface->state &= ~GR_IFACE_S_PROMISC_FIXED;
+				iface->flags &= ~GR_IFACE_F_PROMISC;
+			}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
else
iface->state &= multicast ?
~GR_IFACE_S_ALLMULTI_FIXED :
~GR_IFACE_S_PROMISC_FIXED;
// Reinstall all addresses after disabling allmulti/promisc.
if (multicast) {
ret = rte_eth_dev_set_mc_addr_list(
port->port_id, filter->mac, filter->count
);
} else {
for (i = 0; i < filter->count; i++) {
int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0);
if (r < 0 && ret == 0)
ret = r;
}
}
if (ret < 0)
LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret));
else
if (multicast) {
iface->state &= ~GR_IFACE_S_ALLMULTI_FIXED;
iface->flags &= ~GR_IFACE_F_ALLMULTI;
} else {
iface->state &= ~GR_IFACE_S_PROMISC_FIXED;
iface->flags &= ~GR_IFACE_F_PROMISC;
}
// Reinstall all addresses after disabling allmulti/promisc.
if (multicast) {
ret = rte_eth_dev_set_mc_addr_list(
port->port_id, filter->mac, filter->count
);
} else {
for (i = 0; i < filter->count; i++) {
int r = rte_eth_dev_mac_addr_add(port->port_id, &filter->mac[i], 0);
if (r < 0 && ret == 0)
ret = r;
}
}
if (ret < 0)
LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret));
🤖 Prompt for AI Agents
In modules/infra/control/port.c around lines 674-692, after clearing the forced
state bits (iface->state &= multicast ? ~GR_IFACE_S_ALLMULTI_FIXED :
~GR_IFACE_S_PROMISC_FIXED;), also clear the corresponding iface->flags bit so
the software flags reflect that hardware allmulti/promisc was disabled: if
multicast clear GR_IFACE_F_ALLMULTI from iface->flags, otherwise clear
GR_IFACE_F_PROMISC. Place this update immediately after the state change and
before reinstalling addresses.


if (multicast)
ret = rte_eth_dev_set_mc_addr_list(port->port_id, filter->mac, filter->count);
else
ret = rte_eth_dev_mac_addr_remove(port->port_id, (struct rte_ether_addr *)mac);
} else {
if (multicast)
ret = rte_eth_dev_set_mc_addr_list(
port->port_id, filter->mac, filter->count
);
else
ret = rte_eth_dev_mac_addr_remove(
port->port_id, (struct rte_ether_addr *)mac
);

if (ret < 0)
LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret));
if (ret < 0)
LOG(WARNING, "%s: %s: %s", iface->name, mac_type, rte_strerror(-ret));
}

return 0;
}
Expand Down
13 changes: 13 additions & 0 deletions modules/infra/control/port_priv.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: BSD-3-Clause
// Copyright (c) 2025 Robin Jarry

#pragma once

#include <gr_iface.h>

#include <rte_ether.h>

int port_mac_add(struct iface *, const struct rte_ether_addr *);
int port_mac_del(struct iface *, const struct rte_ether_addr *);
int port_promisc_set(struct iface *, bool enabled);
int port_allmulti_set(struct iface *, bool enabled);
Loading