[v2,3/5] examples/kni: monitor and update link status continually
Checks
Commit Message
Update KNI example to continuously monitor the Ethernet link status of
the physical link and update the carrier status of the corresponding
interfaces with rte_kni_update_link().
Signed-off-by: Dan Gora <dg@adax.com>
---
examples/kni/Makefile | 2 ++
examples/kni/main.c | 82 +++++++++++++++++++++----------------------
2 files changed, 43 insertions(+), 41 deletions(-)
Comments
On 9/19/2018 8:55 PM, Dan Gora wrote:
> Update KNI example to continuously monitor the Ethernet link status of
> the physical link and update the carrier status of the corresponding
> interfaces with rte_kni_update_link().
>
> Signed-off-by: Dan Gora <dg@adax.com>
Also this patch sets kni interfaces "up", please add this information to commit log.
<...>
> + RTE_ETH_FOREACH_DEV(portid) {
> + for (i = 0; i < p[portid]->nb_kni; i++) {
> + name = rte_kni_get_name(p[portid]->kni[i]);
> + snprintf(cmd, sizeof(cmd),
> + "/sbin/ifconfig %s up", name);
> + RTE_LOG(INFO, APP,
> + "Marking interface %s 'up'\n", name);
> + if (system(cmd) == -1)
> + RTE_LOG(ERR, APP,
> + "Error: Failed to mark interface %s 'up'\n",
> + name);
> + }
> + }
This part can be separated thread, overall already done only once, and put into
its own function. And what about making this optional with command line argument.
>
> - printf("\nChecking link status\n");
> - fflush(stdout);
> - for (count = 0; count <= MAX_CHECK_TIME; count++) {
> - all_ports_up = 1;
> + while (kni_running) {
> + rte_delay_ms(CHECK_INTERVAL);
> RTE_ETH_FOREACH_DEV(portid) {
> - if ((port_mask & (1 << portid)) == 0)
> + if ((ports_mask & (1 << portid)) == 0)
> continue;
> memset(&link, 0, sizeof(link));
> rte_eth_link_get_nowait(portid, &link);
> - /* print link status if flag set */
> - if (print_flag == 1) {
> - if (link.link_status)
> - printf(
> - "Port%d Link Up - speed %uMbps - %s\n",
> - portid, link.link_speed,
> - (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> - ("full-duplex") : ("half-duplex\n"));
> - else
> - printf("Port %d Link Down\n", portid);
> - continue;
> - }
> - /* clear all_ports_up flag if any link down */
> - if (link.link_status == ETH_LINK_DOWN) {
> - all_ports_up = 0;
> - break;
> - }
> - }
> - /* after finally printing all link status, get out */
> - if (print_flag == 1)
> - break;
> -
> - if (all_ports_up == 0) {
> - printf(".");
> - fflush(stdout);
> - rte_delay_ms(CHECK_INTERVAL);
> - }
Previous logic makes sure all physical interfaces are up before packet
processing is started. Why removing this logic? I think it is good to keep it as
it is.
<...>
> @@ -947,7 +938,14 @@ main(int argc, char** argv)
>
> kni_alloc(port);
> }
> - check_all_ports_link_status(ports_mask);
> +
> + kni_running = 1;
> + ret = rte_ctrl_thread_create(&kni_link_tid,
> + "KNI link status check", NULL,
> + check_all_ports_link_status, NULL);
This thread is to reflect physical port link status to KNI interface, this can
be useful but not sure to have it as default. Why not introduce this ability,
and creating thread, as an optional feature enabled by command line option?
So overall makes two commandline option,
- one to set kni interfaces up by application
- one to create a thread to check and reflect physical port link status to KNI
interface
On Wed, Sep 26, 2018 at 11:00 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 9/19/2018 8:55 PM, Dan Gora wrote:
>> Update KNI example to continuously monitor the Ethernet link status of
>> the physical link and update the carrier status of the corresponding
>> interfaces with rte_kni_update_link().
>>
>> Signed-off-by: Dan Gora <dg@adax.com>
>
> Also this patch sets kni interfaces "up", please add this information to commit log.
>
> <...>
>
>> + RTE_ETH_FOREACH_DEV(portid) {
>> + for (i = 0; i < p[portid]->nb_kni; i++) {
>> + name = rte_kni_get_name(p[portid]->kni[i]);
>> + snprintf(cmd, sizeof(cmd),
>> + "/sbin/ifconfig %s up", name);
>> + RTE_LOG(INFO, APP,
>> + "Marking interface %s 'up'\n", name);
>> + if (system(cmd) == -1)
>> + RTE_LOG(ERR, APP,
>> + "Error: Failed to mark interface %s 'up'\n",
>> + name);
>> + }
>> + }
>
> This part can be separated thread, overall already done only once, and put into
> its own function. And what about making this optional with command line argument.
>
I'll just remove it.. It's unnecessary. I don't see why the user
should have to explicitly type 'ip link set up dev <name>' external to
the application, but that's the way it was before, so that's fine with
me.
>>
>> - printf("\nChecking link status\n");
>> - fflush(stdout);
>> - for (count = 0; count <= MAX_CHECK_TIME; count++) {
>> - all_ports_up = 1;
>> + while (kni_running) {
>> + rte_delay_ms(CHECK_INTERVAL);
>> RTE_ETH_FOREACH_DEV(portid) {
>> - if ((port_mask & (1 << portid)) == 0)
>> + if ((ports_mask & (1 << portid)) == 0)
>> continue;
>> memset(&link, 0, sizeof(link));
>> rte_eth_link_get_nowait(portid, &link);
>> - /* print link status if flag set */
>> - if (print_flag == 1) {
>> - if (link.link_status)
>> - printf(
>> - "Port%d Link Up - speed %uMbps - %s\n",
>> - portid, link.link_speed,
>> - (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>> - ("full-duplex") : ("half-duplex\n"));
>> - else
>> - printf("Port %d Link Down\n", portid);
>> - continue;
>> - }
>> - /* clear all_ports_up flag if any link down */
>> - if (link.link_status == ETH_LINK_DOWN) {
>> - all_ports_up = 0;
>> - break;
>> - }
>> - }
>> - /* after finally printing all link status, get out */
>> - if (print_flag == 1)
>> - break;
>> -
>> - if (all_ports_up == 0) {
>> - printf(".");
>> - fflush(stdout);
>> - rte_delay_ms(CHECK_INTERVAL);
>> - }
>
> Previous logic makes sure all physical interfaces are up before packet
> processing is started.
Uh, no it didn't. It would wait for up to 9 seconds for the links to
come up and print a message, but if they didn't it would happily just
chug right along.
> Why removing this logic? I think it is good to keep it as it is.
Because it was kind of unnecessary in the first place. Why 9 seconds?
Why not 10? Why not 60? None of the following logic in the program
depends on the link being up.
>> @@ -947,7 +938,14 @@ main(int argc, char** argv)
>>
>> kni_alloc(port);
>> }
>> - check_all_ports_link_status(ports_mask);
>> +
>> + kni_running = 1;
>> + ret = rte_ctrl_thread_create(&kni_link_tid,
>> + "KNI link status check", NULL,
>> + check_all_ports_link_status, NULL);
>
> This thread is to reflect physical port link status to KNI interface, this can
> be useful but not sure to have it as default. Why not introduce this ability,
> and creating thread, as an optional feature enabled by command line option?
>
> So overall makes two commandline option,
> - one to set kni interfaces up by application
> - one to create a thread to check and reflect physical port link status to KNI
> interface
I'm not really inclined to add new command line options here. I'll
remove the code to set the interface 'up' so the user will continue to
have to type 'ip link set up dev <name>' externally the way they do
now. I don't see any reason why checking the link status would
require a command line option. It affects nothing in the datapath.
On 9/26/2018 8:16 PM, Dan Gora wrote:
> On Wed, Sep 26, 2018 at 11:00 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 9/19/2018 8:55 PM, Dan Gora wrote:
>>> Update KNI example to continuously monitor the Ethernet link status of
>>> the physical link and update the carrier status of the corresponding
>>> interfaces with rte_kni_update_link().
>>>
>>> Signed-off-by: Dan Gora <dg@adax.com>
>>
>> Also this patch sets kni interfaces "up", please add this information to commit log.
>>
>> <...>
>>
>>> + RTE_ETH_FOREACH_DEV(portid) {
>>> + for (i = 0; i < p[portid]->nb_kni; i++) {
>>> + name = rte_kni_get_name(p[portid]->kni[i]);
>>> + snprintf(cmd, sizeof(cmd),
>>> + "/sbin/ifconfig %s up", name);
>>> + RTE_LOG(INFO, APP,
>>> + "Marking interface %s 'up'\n", name);
>>> + if (system(cmd) == -1)
>>> + RTE_LOG(ERR, APP,
>>> + "Error: Failed to mark interface %s 'up'\n",
>>> + name);
>>> + }
>>> + }
>>
>> This part can be separated thread, overall already done only once, and put into
>> its own function. And what about making this optional with command line argument.
>>
>
> I'll just remove it.. It's unnecessary. I don't see why the user
> should have to explicitly type 'ip link set up dev <name>' external to
> the application, but that's the way it was before, so that's fine with
> me.
Thanks for the work Dan,
I am trying to be conservative for possible use cases of the sample, as you
pointed before perhaps people would like to use these with kind of network
manager they want to up interface when they want.
I think it was good feature to have but just lets have it optional.
>
>>>
>>> - printf("\nChecking link status\n");
>>> - fflush(stdout);
>>> - for (count = 0; count <= MAX_CHECK_TIME; count++) {
>>> - all_ports_up = 1;
>>> + while (kni_running) {
>>> + rte_delay_ms(CHECK_INTERVAL);
>>> RTE_ETH_FOREACH_DEV(portid) {
>>> - if ((port_mask & (1 << portid)) == 0)
>>> + if ((ports_mask & (1 << portid)) == 0)
>>> continue;
>>> memset(&link, 0, sizeof(link));
>>> rte_eth_link_get_nowait(portid, &link);
>>> - /* print link status if flag set */
>>> - if (print_flag == 1) {
>>> - if (link.link_status)
>>> - printf(
>>> - "Port%d Link Up - speed %uMbps - %s\n",
>>> - portid, link.link_speed,
>>> - (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
>>> - ("full-duplex") : ("half-duplex\n"));
>>> - else
>>> - printf("Port %d Link Down\n", portid);
>>> - continue;
>>> - }
>>> - /* clear all_ports_up flag if any link down */
>>> - if (link.link_status == ETH_LINK_DOWN) {
>>> - all_ports_up = 0;
>>> - break;
>>> - }
>>> - }
>>> - /* after finally printing all link status, get out */
>>> - if (print_flag == 1)
>>> - break;
>>> -
>>> - if (all_ports_up == 0) {
>>> - printf(".");
>>> - fflush(stdout);
>>> - rte_delay_ms(CHECK_INTERVAL);
>>> - }
>>
>> Previous logic makes sure all physical interfaces are up before packet
>> processing is started.
>
> Uh, no it didn't. It would wait for up to 9 seconds for the links to
> come up and print a message, but if they didn't it would happily just
> chug right along.
Right, it will continue after 9 seconds, but it some time to interfaces to be up
before packet processing.
>
>> Why removing this logic? I think it is good to keep it as it is.
>
> Because it was kind of unnecessary in the first place. Why 9 seconds?
> Why not 10? Why not 60? None of the following logic in the program
> depends on the link being up.
I don't think it is unnecessary, this gives some time to interfaces to get
ready. 9 second may be coming from rte_eth_link_get() have 9 second timeout
defined, and I expect that should be coming from possible HW delay, so it make
sense to use that value here.
Anyway if all interfaces are "up" this API should not be a problem at all, I
don't see the motivation to removed it.
>
>>> @@ -947,7 +938,14 @@ main(int argc, char** argv)
>>>
>>> kni_alloc(port);
>>> }
>>> - check_all_ports_link_status(ports_mask);
>>> +
>>> + kni_running = 1;
>>> + ret = rte_ctrl_thread_create(&kni_link_tid,
>>> + "KNI link status check", NULL,
>>> + check_all_ports_link_status, NULL);
>>
>> This thread is to reflect physical port link status to KNI interface, this can
>> be useful but not sure to have it as default. Why not introduce this ability,
>> and creating thread, as an optional feature enabled by command line option?
>>
>> So overall makes two commandline option,
>> - one to set kni interfaces up by application
>> - one to create a thread to check and reflect physical port link status to KNI
>> interface
>
> I'm not really inclined to add new command line options here. I'll
> remove the code to set the interface 'up' so the user will continue to
> have to type 'ip link set up dev <name>' externally the way they do
> now. I don't see any reason why checking the link status would
> require a command line option. It affects nothing in the datapath.
First it is not just checking link status, continuously checking link status on
its own thread, and second reflecting HW link status to virtual interface.
Someone may or may not want both of those, what is the problem of making it
optional?
@@ -20,6 +20,7 @@ static: build/$(APP)-static
PC_FILE := $(shell pkg-config --path libdpdk)
CFLAGS += -O3 $(shell pkg-config --cflags libdpdk)
+CFLAGS += -DALLOW_EXPERIMENTAL_API
LDFLAGS_SHARED = $(shell pkg-config --libs libdpdk)
LDFLAGS_STATIC = -Wl,-Bstatic $(shell pkg-config --static --libs libdpdk)
@@ -54,6 +55,7 @@ please change the definition of the RTE_TARGET environment variable)
endif
CFLAGS += -O3
+CFLAGS += -DALLOW_EXPERIMENTAL_API
CFLAGS += $(WERROR_FLAGS)
include $(RTE_SDK)/mk/rte.extapp.mk
@@ -107,6 +107,8 @@ static uint32_t ports_mask = 0;
/* Ports set in promiscuous mode off by default. */
static int promiscuous_on = 0;
+static int kni_running;
+
/* Structure type for recording kni interface specific stats */
struct kni_interface_stats {
/* number of pkts received from NIC, and sent to KNI */
@@ -620,58 +622,45 @@ init_port(uint16_t port)
}
/* Check the link status of all ports in up to 9s, and print them finally */
-static void
-check_all_ports_link_status(uint32_t port_mask)
+static void *
+check_all_ports_link_status(void *arg)
{
#define CHECK_INTERVAL 100 /* 100ms */
#define MAX_CHECK_TIME 90 /* 9s (90 * 100ms) in total */
uint16_t portid;
- uint8_t count, all_ports_up, print_flag = 0;
struct rte_eth_link link;
+ const char *name;
+ char cmd[64];
+ unsigned int i;
+ struct kni_port_params **p = kni_port_params_array;
+ (void) arg;
+
+ RTE_ETH_FOREACH_DEV(portid) {
+ for (i = 0; i < p[portid]->nb_kni; i++) {
+ name = rte_kni_get_name(p[portid]->kni[i]);
+ snprintf(cmd, sizeof(cmd),
+ "/sbin/ifconfig %s up", name);
+ RTE_LOG(INFO, APP,
+ "Marking interface %s 'up'\n", name);
+ if (system(cmd) == -1)
+ RTE_LOG(ERR, APP,
+ "Error: Failed to mark interface %s 'up'\n",
+ name);
+ }
+ }
- printf("\nChecking link status\n");
- fflush(stdout);
- for (count = 0; count <= MAX_CHECK_TIME; count++) {
- all_ports_up = 1;
+ while (kni_running) {
+ rte_delay_ms(CHECK_INTERVAL);
RTE_ETH_FOREACH_DEV(portid) {
- if ((port_mask & (1 << portid)) == 0)
+ if ((ports_mask & (1 << portid)) == 0)
continue;
memset(&link, 0, sizeof(link));
rte_eth_link_get_nowait(portid, &link);
- /* print link status if flag set */
- if (print_flag == 1) {
- if (link.link_status)
- printf(
- "Port%d Link Up - speed %uMbps - %s\n",
- portid, link.link_speed,
- (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
- ("full-duplex") : ("half-duplex\n"));
- else
- printf("Port %d Link Down\n", portid);
- continue;
- }
- /* clear all_ports_up flag if any link down */
- if (link.link_status == ETH_LINK_DOWN) {
- all_ports_up = 0;
- break;
- }
- }
- /* after finally printing all link status, get out */
- if (print_flag == 1)
- break;
-
- if (all_ports_up == 0) {
- printf(".");
- fflush(stdout);
- rte_delay_ms(CHECK_INTERVAL);
- }
-
- /* set the print_flag if all ports up or timeout */
- if (all_ports_up == 1 || count == (MAX_CHECK_TIME - 1)) {
- print_flag = 1;
- printf("done\n");
+ for (i = 0; i < p[portid]->nb_kni; i++)
+ rte_kni_update_link(p[portid]->kni[i], &link);
}
}
+ return NULL;
}
/* Callback for request of changing MTU */
@@ -893,6 +882,8 @@ main(int argc, char** argv)
int ret;
uint16_t nb_sys_ports, port;
unsigned i;
+ void *retval;
+ pthread_t kni_link_tid;
/* Associate signal_hanlder function with USR signals */
signal(SIGUSR1, signal_handler);
@@ -947,7 +938,14 @@ main(int argc, char** argv)
kni_alloc(port);
}
- check_all_ports_link_status(ports_mask);
+
+ kni_running = 1;
+ ret = rte_ctrl_thread_create(&kni_link_tid,
+ "KNI link status check", NULL,
+ check_all_ports_link_status, NULL);
+ if (ret < 0)
+ rte_exit(EXIT_FAILURE,
+ "Could not create link status thread!\n");
/* Launch per-lcore function on every lcore */
rte_eal_mp_remote_launch(main_loop, NULL, CALL_MASTER);
@@ -955,6 +953,8 @@ main(int argc, char** argv)
if (rte_eal_wait_lcore(i) < 0)
return -1;
}
+ kni_running = 0;
+ pthread_join(kni_link_tid, &retval);
/* Release resources */
RTE_ETH_FOREACH_DEV(port) {