kni: add API to set link status on kernel interface

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

Quoted 5 lines · Dan Gora

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

+1 to sysfs implementation.

But right now this API is not used at all which makes it hard to test and catch when API broken.
Can you please implement the API either on kni sample app or kni unit test?

Also you need to add new API to .map file for shared library build. (you would catch this if API implemented somewhere...)

Set the carrier state to 'off' when the interface is instantiated or when it is marked 'up' or 'down'. This is necessary to set the interface to a known operational state until the carrier state is changed with rte_kni_update_link().

Signed-off-by Dan Gora <[email protected]>

---
 kernel/linux/kni/kni_misc.c | 2 ++
 kernel/linux/kni/kni_net.c  | 2 ++
 2 files changed, 4 insertions(+)

kernel/linux/kni/kni_misc.c +2 -0

index fa69f8e63..45649499d 100644
@@ -466,6 +466,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		return -ENODEV;
 	}
 
+	netif_carrier_off(net_dev);
+
 	ret = kni_run_thread(knet, kni, dev_info.force_bind);
 	if (ret != 0)
 		return ret;

kernel/linux/kni/kni_net.c +2 -0

index 7fcfa106c..1f8ba0700 100644
@@ -133,6 +133,7 @@ kni_net_open(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_start_queue(dev);
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;

@@ -152,6 +153,7 @@ kni_net_release(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_stop_queue(dev); /* can't transmit any more */
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;

Quoted 4 lines · Dan Gora

Set the carrier state to 'off' when the interface is instantiated or when it is marked 'up' or 'down'. This is necessary to set the interface to a known operational state until the carrier state is changed with rte_kni_update_link().

Why setting to no-carrier mode by default? This will change the behavior of interfaces and may effect others. And indeed I didn't get why this is required?

Quoted 42 lines

Signed-off-by Dan Gora <[email protected]>

---
 kernel/linux/kni/kni_misc.c | 2 ++
 kernel/linux/kni/kni_net.c  | 2 ++
 2 files changed, 4 insertions(+)

kernel/linux/kni/kni_misc.c +2 -0

index fa69f8e63..45649499d 100644
@@ -466,6 +466,8 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
 		return -ENODEV;
 	}
 
+	netif_carrier_off(net_dev);
+
 	ret = kni_run_thread(knet, kni, dev_info.force_bind);
 	if (ret != 0)
 		return ret;

kernel/linux/kni/kni_net.c +2 -0

index 7fcfa106c..1f8ba0700 100644
@@ -133,6 +133,7 @@ kni_net_open(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_start_queue(dev);
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;

@@ -152,6 +153,7 @@ kni_net_release(struct net_device *dev)
 	struct kni_dev *kni = netdev_priv(dev);
 
 	netif_stop_queue(dev); /* can't transmit any more */
+	netif_carrier_off(dev);
 
 	memset(&req, 0, sizeof(req));
 	req.req_id = RTE_KNI_REQ_CFG_NETWORK_IF;

I just read the other thread, including Igor's and your comment about starting the interface down, overall I got your point but my concerns is if someone has a solution based on assumption that interface will be up when created will be affected.

Hi All,

The following patches are to add support for DPDK applications to be able to change the carrier state of Linux network interfaces in the KNI kernel module.

The carrier state is changed by writing to the Linux /sys file:
/sys/devices/virtual/net/<name>/carrier, where <name> is the KNI interface name.

These patches supercede:
'[PATCH v2 10/10] kni: add API to set link status on kernel interface'
https://mails.dpdk.org/archives/dev/2018-August/110383.html

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

lib/librte_kni/rte_kni.c +35 -0

+int __rte_experimental
+rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link)
+{
+	char path[64];
+	char carrier[2];
+	const char *new_carrier;
+	int fd, ret;
+
+	if (kni == NULL || link == NULL)
+		return -1;
+
+	snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
+		kni->name);
+
+	fd = open(path, O_RDWR);
+	if (fd == -1) {
+		RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
+		return -1;
+	}
+
+	ret = read(fd, carrier, 2);
+	if (ret < 1) {
+		/* Cannot read carrier until interface is marked
+		 * 'up', so don't log an error.
+		 */
+		close(fd);
+		return -1;
+	}
+
+	new_carrier = (link->link_status == ETH_LINK_UP) ? "1" : "0";
+	ret = write(fd, new_carrier, 1);
+	if (ret < 1) {
+		RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
+		close(fd);
+		return -1;

Quoted 5 lines · Dan Gora

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

<...>

Quoted 41 lines

lib/librte_kni/rte_kni.c +41 -0

+int __rte_experimental
+rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link)
+{
+     char path[64];
+     char carrier[2];
+     const char *new_carrier;
+     int fd, ret;
+
+     if (kni == NULL || link == NULL)
+             return -1;
+
+     snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
+             kni->name);
+
+     fd = open(path, O_RDWR);
+     if (fd == -1) {
+             RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
+             return -1;
+     }
+
+     ret = read(fd, carrier, 2);
+     if (ret < 1) {
+             /* Cannot read carrier until interface is marked
+              * 'up', so don't log an error.
+              */
+             close(fd);
+             return -1;
+     }
+
+     new_carrier = (link->link_status == ETH_LINK_UP) ? "1" : "0";
+     ret = write(fd, new_carrier, 1);
+     if (ret < 1) {
+             RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
+             close(fd);
+             return -1;
+     }
+
+     if (strncmp(carrier, new_carrier, 1)) {
+             if (link->link_status == ETH_LINK_UP) {
+                     RTE_LOG(INFO, KNI, "%s NIC Link is Up %d Mbps %s %s.\n",
+                             kni->name,

lib/librte_kni/rte_kni.c rte_kni_unregister_handlers(struct rte_kni *kni) · line 838

Quoted 4 lines

lib/librte_kni/rte_kni.c +4 -0

+                             link->link_speed,
+                             link->link_autoneg ? "(AutoNeg)" : "(Fixed)",
+                             link->link_duplex ?
+                                     "Full Duplex" : "Half Duplex");

These link values are coming from user and not reflected to the kni interface, printing them here can cause a misunderstanding that they have been applied. I think only link status should be printed here.

<...>

test/test/test_kni.c test_kni_loop(__rte_unused void *arg) · line 247

Quoted 10 lines

test/test/test_kni.c +6 -0

@@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
                      ret = -1;
              if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
                      ret = -1;
+
+             ret = kni_change_link();
+             if (ret != 0) {
+                     test_kni_processing_flag = 1;
+                     return ret;
+             }

I thinks this is wrong place to call kni_change_link(), this is test_kni_loop() created by test_kni_processing() that does packet processing tests.

I think better to call directly from test_kni(), perhaps before test_kni_processing() call?

Quoted 50 lines · Ferruh Yigit

Quoted 5 lines · Dan Gora

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

<...>

Quoted 41 lines

lib/librte_kni/rte_kni.c +41 -0

+int __rte_experimental
+rte_kni_update_link(struct rte_kni *kni, struct rte_eth_link *link)
+{
+     char path[64];
+     char carrier[2];
+     const char *new_carrier;
+     int fd, ret;
+
+     if (kni == NULL || link == NULL)
+             return -1;
+
+     snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
+             kni->name);
+
+     fd = open(path, O_RDWR);
+     if (fd == -1) {
+             RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
+             return -1;
+     }
+
+     ret = read(fd, carrier, 2);
+     if (ret < 1) {
+             /* Cannot read carrier until interface is marked
+              * 'up', so don't log an error.
+              */
+             close(fd);
+             return -1;
+     }
+
+     new_carrier = (link->link_status == ETH_LINK_UP) ? "1" : "0";
+     ret = write(fd, new_carrier, 1);
+     if (ret < 1) {
+             RTE_LOG(ERR, KNI, "Failed to write file: %s.\n", path);
+             close(fd);
+             return -1;
+     }
+
+     if (strncmp(carrier, new_carrier, 1)) {
+             if (link->link_status == ETH_LINK_UP) {
+                     RTE_LOG(INFO, KNI, "%s NIC Link is Up %d Mbps %s %s.\n",
+                             kni->name,

Quoted 9 lines

lib/librte_kni/rte_kni.c rte_kni_unregister_handlers(struct rte_kni *kni) · line 838

Quoted 4 lines

lib/librte_kni/rte_kni.c +4 -0

+                             link->link_speed,
+                             link->link_autoneg ? "(AutoNeg)" : "(Fixed)",
+                             link->link_duplex ?
+                                     "Full Duplex" : "Half Duplex");

These link values are coming from user and not reflected to the kni interface, printing them here can cause a misunderstanding that they have been applied. I think only link status should be printed here.

There is nothing to "reflect" to the kernel interface, nor to apply to the kernel interface. This is exactly how every other kernel driver works on link status changes. There is no "netif_set_speed()' function. When a link status change occurs the kernel driver calls netif_carrier_on/off() and prints a message like this one.

Quoted 17 lines

<...>

test/test/test_kni.c test_kni_loop(__rte_unused void *arg) · line 247

Quoted 10 lines

test/test/test_kni.c +6 -0

@@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
                      ret = -1;
              if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
                      ret = -1;
+
+             ret = kni_change_link();
+             if (ret != 0) {
+                     test_kni_processing_flag = 1;
+                     return ret;
+             }

I thinks this is wrong place to call kni_change_link(), this is test_kni_loop() created by test_kni_processing() that does packet processing tests.

Well, no it's not "wrong". The interface has to be up to change the link state, so this is a convenient place to do it.

Quoted 3 lines

I think better to call directly from test_kni(), perhaps before test_kni_processing() call?

Because then we'd have to add code to set the interface up and down again, and there really is no point. This is as good a place as any. It does not affect the data transfer tests at all.

-d

Quoted 5 lines

There is nothing to "reflect" to the kernel interface, nor to apply to the kernel interface. This is exactly how every other kernel driver works on link status changes. There is no "netif_set_speed()' function. When a link status change occurs the kernel driver calls netif_carrier_on/off() and prints a message like this one.

I am not suggesting reflecting these into interface, I am just saying why do you print them?

For example, "link->link_speed" this is coming as parameter to API, this API does nothing with this value, why print is here?

I assume you are using "rte_eth_link" as parameter, instead of basic "link_status" to make it extendible in the feature. If so please print those other values when function extended, right now only link_status matters.

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

test/test/test_kni.c +1 -0

@@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
                      ret = -1;
              if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
                      ret = -1;
+

Quoted 9 lines

test/test/test_kni.c test_kni_loop(__rte_unused void *arg) · line 247

Quoted 5 lines

test/test/test_kni.c +5 -0

+             ret = kni_change_link();
+             if (ret != 0) {
+                     test_kni_processing_flag = 1;
+                     return ret;
+             }

I thinks this is wrong place to call kni_change_link(), this is test_kni_loop() created by test_kni_processing() that does packet processing tests.

Quoted 2 lines

Well, no it's not "wrong". The interface has to be up to change the link state, so this is a convenient place to do it.

Are we agree that this function in unit test is to test packet processing part of KNI?
And for example please check following coming patch:
https://patches.dpdk.org/patch/44730/

I think you want to benefit from "system(IFCONFIG TEST_KNI_PORT" up")" since interface needs to be up to set the link, but you can do same call, not have to use that one.

Quoted 3 lines

Because then we'd have to add code to set the interface up and down again, and there really is no point. This is as good a place as any. It does not affect the data transfer tests at all.

Doesn't affect the data transfer, agreed, but why confusing data transfer test with link up/down calls? Why not have your function and set interface up and down again?

Quoted 6 lines · Ferruh Yigit

Quoted 5 lines

There is nothing to "reflect" to the kernel interface, nor to apply to the kernel interface. This is exactly how every other kernel driver works on link status changes. There is no "netif_set_speed()' function. When a link status change occurs the kernel driver calls netif_carrier_on/off() and prints a message like this one.

Quoted 2 lines

I am not suggesting reflecting these into interface, I am just saying why do you print them?

Because the information is useful and because every other Ethernet driver does the same thing when the link status changes.

See the Linux kernel:

ixgbe_watchdog_link_is_up(): drivers/net/ethernet/intel/ixgbe/ixgbe_main.c print_link_info(): drivers/net/ethernet/cavium/liquidio/lio_main.c bnx2_report_link(): drivers/net/ethernet/broadcom/bnx2.c

and dozens of other examples.

Imagine you plug your 1G ethernet cable into a 10/100Mbps hub. Yes the link will be up, but it's the wrong speed.
Imagine you had accidentally forced your 1G connection to 10Mbps fixed. It will only come up at 10Mbps. Wouldn't you like to know that autoNeg was disabled?

Quoted 2 lines

For example, "link->link_speed" this is coming as parameter to API, this API does nothing with this value, why print is here?

Because wouldn't you like to know if your link has connected at the correct speed? I would.

Quoted 3 lines

I assume you are using "rte_eth_link" as parameter, instead of basic "link_status" to make it extendible in the feature. If so please print those other values when function extended, right now only link_status matters.

No, all that information matters. If you have autoNeg disabled or are connecting to a broken piece of equipment, the link can come up but be at the wrong speed.

There is not much to extend this function with. The only action we can take, other than to print the information, is to write to the /sys file to have the KNI kernel module call netif_carrier_on/off() for us.

Quoted 18 lines

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

test/test/test_kni.c +1 -0

@@ -148,9 +239,16 @@ test_kni_loop(__rte_unused void *arg)
                      ret = -1;
              if (system(IFCONFIG TEST_KNI_PORT" mtu 1400") == -1)
                      ret = -1;
+

Quoted 9 lines

test/test/test_kni.c test_kni_loop(__rte_unused void *arg) · line 247

Quoted 5 lines

test/test/test_kni.c +5 -0

+             ret = kni_change_link();
+             if (ret != 0) {
+                     test_kni_processing_flag = 1;
+                     return ret;
+             }

I thinks this is wrong place to call kni_change_link(), this is test_kni_loop() created by test_kni_processing() that does packet processing tests.

Quoted 2 lines

Well, no it's not "wrong". The interface has to be up to change the link state, so this is a convenient place to do it.

Quoted 8 lines

Are we agree that this function in unit test is to test packet processing part of KNI?
And for example please check following coming patch:
https://patches.dpdk.org/patch/44730/

I think you want to benefit from "system(IFCONFIG TEST_KNI_PORT" up")" since interface needs to be up to set the link, but you can do same call, not have to use that one.

ok, I'll fix this. Should I rebase the changes on top of this patch
(https://patches.dpdk.org/patch/44730/)? Is that patch going to get accepted soon?

Quoted 7 lines

Quoted 3 lines

Because then we'd have to add code to set the interface up and down again, and there really is no point. This is as good a place as any. It does not affect the data transfer tests at all.

Doesn't affect the data transfer, agreed, but why confusing data transfer test with link up/down calls? Why not have your function and set interface up and down again?

ok. I was just trying to minimize the number of changes. I'll send a new patch.

Quoted 7 lines · Dan Gora

Quoted 6 lines · Ferruh Yigit

Quoted 5 lines

There is nothing to "reflect" to the kernel interface, nor to apply to the kernel interface. This is exactly how every other kernel driver works on link status changes. There is no "netif_set_speed()' function. When a link status change occurs the kernel driver calls netif_carrier_on/off() and prints a message like this one.

Quoted 5 lines

Quoted 2 lines

I am not suggesting reflecting these into interface, I am just saying why do you print them?

Because the information is useful and because every other Ethernet driver does the same thing when the link status changes.

It would be useful if it writes the values of virtual interface, but this API prints user input.

The virtual interface may have different value, this API doesn't change anything related other than link status, so why print user provided value.

Won't you think it will be confusing if the virtual interface values are different than what printed.

Or won't user will think API changed those values to printed one in interface?

Quoted 12 lines · Ferruh Yigit

Quoted 10 lines · Dan Gora

Quoted 8 lines · Ferruh Yigit

Quoted 5 lines

There is nothing to "reflect" to the kernel interface, nor to apply to the kernel interface. This is exactly how every other kernel driver works on link status changes. There is no "netif_set_speed()' function. When a link status change occurs the kernel driver calls netif_carrier_on/off() and prints a message like this one.

I am not suggesting reflecting these into interface, I am just saying why do you print them?

Quoted 6 lines

Quoted 2 lines

Because the information is useful and because every other Ethernet driver does the same thing when the link status changes.

It would be useful if it writes the values of virtual interface, but this API prints user input.

I'm sorry, Ferruh, I really don't understand what you are referring to here. What virtual interface are you talking about?

Quoted 3 lines

The virtual interface may have different value, this API doesn't change anything related other than link status, so why print user provided value.

Again, because all this information is useful if the KNI interface is reflecting the state of a real physical ethenet interface. If the KNI interface is not reflecting a real peice of hardware, the user can continue not using this new API function or do something like set the speed to 0. This is what we get from ENA PMD drivers on amazon for instance. It's perfectly reasonable.

Quoted 3 lines

Won't you think it will be confusing if the virtual interface values are different than what printed.

If what values are different from what? I'm really confused as to what use case you are referring to here. Can you give a concrete example?

Quoted 1 line

Or won't user will think API changed those values to printed one in interface?

again, what values? Changed how?

Quoted 5 lines

Quoted 3 lines

It would be useful if it writes the values of virtual interface, but this API prints user input.

I'm sorry, Ferruh, I really don't understand what you are referring to here. What virtual interface are you talking about?

Virtual interface is KNI interface, Linux virtual network interface.

Let me try again,

You are adding a new API to KNI library, which is to set link status of KNI interface.

This API prints some link related values in its log. But these values are not applied to KNI interface or not the values coming from KNI interface. These are just values provided by user to the API, I am saying printing these values in log can be confusing, the user may think API applies these values to KNI interface.

Quoted 14 lines · Ferruh Yigit

Quoted 5 lines

Quoted 3 lines

It would be useful if it writes the values of virtual interface, but this API prints user input.

I'm sorry, Ferruh, I really don't understand what you are referring to here. What virtual interface are you talking about?

Virtual interface is KNI interface, Linux virtual network interface.

Let me try again,

You are adding a new API to KNI library, which is to set link status of KNI interface.

Quoted 4 lines

This API prints some link related values in its log. But these values are not applied to KNI interface or not the values coming from KNI interface. These are just values provided by user to the API, I am saying printing these values in log can be confusing, the user may think API applies these values to KNI interface.

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

Again, this is how every other
Ethernet driver in linux works. I doubt that someone would be confused by that.

If the KNI interface is purely virtual (that is, it does not correspond to any physical Ethernet port), then the DPDK application writer is under no obligation to use this API function, or they can use it and just set the speed to 0. If the end user understands that the KNI interface is purely virtual, then they should not be confused that the speed is 0 or that the duplex/autoNeg values are irrelevant.

I just don't see how this could cause any confusion.

Quoted 3 lines

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

+1 to this, people would like to know link speed of the interface. Are you printing link speed of interface? You are printing whatever user pass to API.

I guess you trust to user to provide correct values there, but since only link up & down matters, what prevents user to leave other fields, like speed, just random values?

Quoted 6 lines · Ferruh Yigit

Quoted 3 lines

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

+1 to this, people would like to know link speed of the interface. Are you printing link speed of interface? You are printing whatever user

pass to

Quoted 1 line

API.

There is no such thing as "link speed of the interface". The link speed is the speed of the underlying Ethernet link that the interface corresponds to. This is true for all other ethernet interfaces in the kernel.

Quoted 1 line

I guess you trust to user to provide correct values there, but since

only link

Quoted 1 line

up & down matters, what prevents user to leave other fields, like speed,

just

Quoted 1 line

random values?

Nothing. What prevents anyone from providing random values for anything? The point of the API was to make it super simple, just:

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

No messing around with the link info retrieved from rte_eth_link_get(_nowait), just dump in the struct that was returned.

Quoted 4 lines · Ferruh Yigit

Quoted 3 lines

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

Quoted 3 lines

+1 to this, people would like to know link speed of the interface. Are you printing link speed of interface? You are printing whatever user pass to API.

There is no such thing as "link speed of the interface". The link speed is the speed of the underlying Ethernet link that the interface corresponds to. This is true for all other ethernet interfaces in the kernel.

Quoted 4 lines

I guess you trust to user to provide correct values there, but since only link up & down matters, what prevents user to leave other fields, like speed, just random values?

Nothing. What prevents anyone from providing random values for anything? The point of the API was to make it super simple, just:

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

No messing around with the link info retrieved from rte_eth_link_get(_nowait), just dump in the struct that was returned.

Quoted 9 lines · Dan Gora

Quoted 7 lines · Ferruh Yigit

Quoted 3 lines

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

+1 to this, people would like to know link speed of the interface. Are you printing link speed of interface? You are printing whatever user pass to API.

Quoted 4 lines

There is no such thing as "link speed of the interface". The link speed is the speed of the underlying Ethernet link that the interface corresponds to. This is true for all other ethernet interfaces in the kernel.

This is an API to set link status of KNI interface, KNI interface is a virtual interface and no need to be backed by a physical interface at all.

Only kni sample application uses it in a way to match a physical interface to a KNI interface, but please check KNI PMD where you can have multiple KNI interface without any physical device at all.

Quoted 7 lines

Quoted 3 lines

I guess you trust to user to provide correct values there, but since only link up & down matters, what prevents user to leave other fields, like speed, just random values?

Nothing. What prevents anyone from providing random values for anything? The point of the API was to make it super simple, just:

Quoted 2 lines

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

You are only thinking about this use case. Normally the input to API should be verified right, for this case there is no way to verify it and vales are not used at all, it is just printed in API.

Quoted 11 lines · Ferruh Yigit

Quoted 9 lines · Dan Gora

Quoted 7 lines · Ferruh Yigit

Quoted 3 lines

Well, yes the link_status (link up, link down) _is_ applied to the KNI interface. When that occurs, most people want to know what the link speed is that the link came up at.

+1 to this, people would like to know link speed of the interface. Are you printing link speed of interface? You are printing whatever user pass to API.

Quoted 7 lines

Quoted 4 lines

There is no such thing as "link speed of the interface". The link speed is the speed of the underlying Ethernet link that the interface corresponds to. This is true for all other ethernet interfaces in the kernel.

This is an API to set link status of KNI interface, KNI interface is a virtual interface and no need to be backed by a physical interface at all.

Yes, I understand that.

Quoted 3 lines

Only kni sample application uses it in a way to match a physical interface to a KNI interface, but please check KNI PMD where you can have multiple KNI interface without any physical device at all.

Yes, I understand that.. As I pointed out previously, if there is no physical device which corresponds to this KNI interface, the application can:

1) Not use this API at all, just as they do now.

2) Use the API and set the state to linkup/0Mbps/autoNeg/Full. This is perfectly reasonable and no one would be confused by this.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

I guess you trust to user to provide correct values there, but since only link up & down matters, what prevents user to leave other fields, like speed, just random values?

Nothing. What prevents anyone from providing random values for anything? The point of the API was to make it super simple, just:

Quoted 6 lines

Quoted 2 lines

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

You are only thinking about this use case. Normally the input to API should be verified right, for this case there is no way to verify it and vales are not used at all, it is just printed in API.

In most applications it is useful to have a message printed which shows the *change* in link status as well as the speed that the link came up at. If you don't provide the link speed, etc information, then the API is not really useful. Yes the application writer can provide whatever they want for the link speed/duplex/autoneg, but so what? Their application will have a nonsensical log message. It's not DPDK's job to enforce sanity on everyone's application.

And no, not every input to every API function is verified. There are lots of examples in the DPDK where this is not the case. The parameters should be verified to ensure that they do not cause the application to "break", not necessarily so that they make sense (whatever that would mean in this case).

It was recommended that we change the rte_kni_update_link() API to only use the link status as an input and print the log messages outside the API in the application code. However think about what that would entail:

You only want to log *changes* in the link state, not log a message when you set the same state. This means that either:

1) the application would have to store the previous carrier state.
2) rte_kni_update_link could return the previous value read from the /sys/.../carrier file.
3) the application could read the /sys/.../carrier file before calling rte_kni_update_link to read the previous carrier state.

In case 1 you're obliging all users of this API to carry around this extra state for no real reason. The API can easily read the /sys/.../carrier file to see the previous state while it has it open, just as it does in this patch.

In case 2, the application can easily detect when the state changes, but then you end up with a return value something like -1 for errors, 0 for previous_link == down, 1 for previous_link == up. I fail to see how this is a better API design that what has already been proposed, but it's at least somewhat useful, even if awkward.

In the DPDK application, you get something like:

rte_eth_link_get_nowait(portid, &link);
prev_link = rte_kni_update_link(p[portid]->kni[i], link.link_status); if (prev_link >= 0)

{
   if (prev_link == 0 && link.link_status == 1)
     printf("LINKUP: speed %d duplex %s autoneg %s\n", ....)
  else if (prev_link == 1 && link.link_status == 0)
    printf("LINKDOWN:\n");
} else {

printf("Error: something went wrong...\n");
}

I don't really see how this is better than:

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

but that's my opinion...

In case 3, you might as well not even have this API since you're duplicating half of the same code.

I would be willing to implement case 2, however I still think that it is not as good a design and will unnecessarily complicate application code compared to the current patch here. Cases 1 and 3 are non-starters for me. Our application needs log messages only when the link *changes* state. These log messages need to include the link speed/duplex/autoneg info. I think that most, if not all, applications would want that too. It's how every other physical ethernet driver in the linux kernel works. DPDK applications which use only KNI virtual interfaces without a physical interface are under no obligation to use this API function.

thanks
dan

Quoted 5 lines

Yes, I understand that.. As I pointed out previously, if there is no physical device which corresponds to this KNI interface, the application can:

1) Not use this API at all, just as they do now.

This API has nothing with if KNI interface has corresponding physical device or not, all KNI users can use it.

Quoted 2 lines

2) Use the API and set the state to linkup/0Mbps/autoNeg/Full. This is perfectly reasonable and no one would be confused by this.

That is the think, you are not setting anything, you are just printing "0Mbps/autoNeg/Full" and assuming/hoping the _if_ there is a corresponding physical device app sends the values of that device to API for printing.

Overall I am not sure if there is a value to discuss this more, and I don't want this relatively small issue to block the patch, I will comment more to latest version.

Quoted 90 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

I guess you trust to user to provide correct values there, but since only link up & down matters, what prevents user to leave other fields, like speed, just random values?

Nothing. What prevents anyone from providing random values for anything? The point of the API was to make it super simple, just:

Quoted 6 lines

Quoted 2 lines

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

You are only thinking about this use case. Normally the input to API should be verified right, for this case there is no way to verify it and vales are not used at all, it is just printed in API.

In most applications it is useful to have a message printed which shows the *change* in link status as well as the speed that the link came up at. If you don't provide the link speed, etc information, then the API is not really useful. Yes the application writer can provide whatever they want for the link speed/duplex/autoneg, but so what? Their application will have a nonsensical log message. It's not DPDK's job to enforce sanity on everyone's application.

And no, not every input to every API function is verified. There are lots of examples in the DPDK where this is not the case. The parameters should be verified to ensure that they do not cause the application to "break", not necessarily so that they make sense (whatever that would mean in this case).

It was recommended that we change the rte_kni_update_link() API to only use the link status as an input and print the log messages outside the API in the application code. However think about what that would entail:

You only want to log *changes* in the link state, not log a message when you set the same state. This means that either:

1) the application would have to store the previous carrier state.
2) rte_kni_update_link could return the previous value read from the /sys/.../carrier file.
3) the application could read the /sys/.../carrier file before calling rte_kni_update_link to read the previous carrier state.

In case 1 you're obliging all users of this API to carry around this extra state for no real reason. The API can easily read the /sys/.../carrier file to see the previous state while it has it open, just as it does in this patch.

In case 2, the application can easily detect when the state changes, but then you end up with a return value something like -1 for errors, 0 for previous_link == down, 1 for previous_link == up. I fail to see how this is a better API design that what has already been proposed, but it's at least somewhat useful, even if awkward.

In the DPDK application, you get something like:

rte_eth_link_get_nowait(portid, &link);
prev_link = rte_kni_update_link(p[portid]->kni[i], link.link_status); if (prev_link >= 0)

{
   if (prev_link == 0 && link.link_status == 1)
     printf("LINKUP: speed %d duplex %s autoneg %s\n", ....)
  else if (prev_link == 1 && link.link_status == 0)
    printf("LINKDOWN:\n");
} else {

printf("Error: something went wrong...\n");
}

I don't really see how this is better than:

rte_eth_link_get_nowait(portid, &link);
rte_kni_update_link(p[portid]->kni[i], &link);

but that's my opinion...

In case 3, you might as well not even have this API since you're duplicating half of the same code.

I would be willing to implement case 2, however I still think that it is not as good a design and will unnecessarily complicate application code compared to the current patch here. Cases 1 and 3 are non-starters for me. Our application needs log messages only when the link *changes* state. These log messages need to include the link speed/duplex/autoneg info. I think that most, if not all, applications would want that too. It's how every other physical ethernet driver in the linux kernel works. DPDK applications which use only KNI virtual interfaces without a physical interface are under no obligation to use this API function.

thanks
dan

Set the carrier state to 'off' when the interface is instantiated or when it is marked 'up' or 'down'. This is necessary to set the interface to a known operational state until the carrier state is changed with rte_kni_update_link().

Signed-off-by Dan Gora <[email protected]>

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 <[email protected]>

examples/kni/main.c +3 -22

-	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;
-			}

Quoted 5 lines · Dan Gora

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 <[email protected]>

Also this patch sets kni interfaces "up", please add this information to commit log.

<...>

examples/kni/main.c init_port(uint16_t port) · line 650

Quoted 13 lines

examples/kni/main.c +13 -0

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

Quoted 30 lines

examples/kni/main.c +3 -22

-     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;
-                     }

examples/kni/main.c init_port(uint16_t port) · line 667

Quoted 10 lines

examples/kni/main.c +0 -10

-             }
-             /* 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.

examples/kni/main.c main(int argc, char** argv) · line 945

Quoted 10 lines

examples/kni/main.c +5 -1

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

Quoted 25 lines · Ferruh Yigit

Quoted 5 lines · Dan Gora

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 <[email protected]>

Also this patch sets kni interfaces "up", please add this information to commit log.

<...>

Quoted 13 lines

examples/kni/main.c +13 -0

+     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);
+             }
+     }

Quoted 3 lines

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.

Quoted 42 lines

Quoted 41 lines

examples/kni/main.c +3 -32

-     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);
-             }

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 16 lines

examples/kni/main.c main(int argc, char** argv) · line 945

Quoted 11 lines

examples/kni/main.c +5 -1

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

Quoted 4 lines

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.

Add logging messages showing the commands necessary for the user to have the application display and zero the statistics.

Signed-off-by Dan Gora <[email protected]>

The worker threads incrementing the rx/tx_packets race with the signal handler from the main thread zeroing the entire statistics structure. This can cause the statistics to fail to be zeroed, even when there is no traffic on those interfaces.

Improve zeroing the statistics by only incrementing rx/tx_packets in worker threads by a non-zero amount. This limits the race to the periods in which traffic is actually being received or transmitted.

Signed-off-by Dan Gora <[email protected]>

---
 examples/kni/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

examples/kni/main.c +4 -2

index ca45347d8..d00569740 100644
@@ -223,7 +223,8 @@ kni_ingress(struct kni_port_params *p)
 		}
 		/* Burst tx to kni */
 		num = rte_kni_tx_burst(p->kni[i], pkts_burst, nb_rx);
-		kni_stats[port_id].rx_packets += num;
+		if (num)
+			kni_stats[port_id].rx_packets += num;
 
 		rte_kni_handle_request(p->kni[i]);
 		if (unlikely(num < nb_rx)) {

@@ -260,7 +261,8 @@ kni_egress(struct kni_port_params *p)
 		}
 		/* Burst tx to eth */
 		nb_tx = rte_eth_tx_burst(port_id, 0, pkts_burst, (uint16_t)num);
-		kni_stats[port_id].tx_packets += nb_tx;
+		if (nb_tx)
+			kni_stats[port_id].tx_packets += nb_tx;
 		if (unlikely(nb_tx < num)) {
 			/* Free mbufs not tx to NIC */
 			kni_burst_free_mbufs(&pkts_burst[nb_tx], num - nb_tx);

Quoted 5 lines · Dan Gora

The worker threads incrementing the rx/tx_packets race with the signal handler from the main thread zeroing the entire statistics structure. This can cause the statistics to fail to be zeroed, even when there is no traffic on those interfaces.

Quoted 3 lines

Improve zeroing the statistics by only incrementing rx/tx_packets in worker threads by a non-zero amount. This limits the race to the periods in which traffic is actually being received or transmitted.

Not sure about introducing an extra check to datapath for possible error on stats zero. I am for dropping this patch, what do you think?

Quoted 6 lines · Ferruh Yigit

Quoted 5 lines · Dan Gora

The worker threads incrementing the rx/tx_packets race with the signal handler from the main thread zeroing the entire statistics structure. This can cause the statistics to fail to be zeroed, even when there is no traffic on those interfaces.

Quoted 6 lines

Quoted 3 lines

Improve zeroing the statistics by only incrementing rx/tx_packets in worker threads by a non-zero amount. This limits the race to the periods in which traffic is actually being received or transmitted.

Not sure about introducing an extra check to datapath for possible error on stats zero. I am for dropping this patch, what do you think?

This is literally adding one instruction to the datapath. Not even an atomic instruction. There is no effect on the performance caused by this change.

Is that not better than the user (like me who experienced this) wondering why they cannot zero the counters even when there is no traffic?

Quoted 7 lines

Quoted 2 lines

Not sure about introducing an extra check to datapath for possible error on stats zero. I am for dropping this patch, what do you think?

This is literally adding one instruction to the datapath. Not even an atomic instruction. There is no effect on the performance caused by this change.

Quoted 3 lines

Is that not better than the user (like me who experienced this) wondering why they cannot zero the counters even when there is no traffic?

Can we have something like, stop the forwarding, clear the stats and start forwarding back?

Yes effect is small but it is for each packet, it doesn't make sense to me to add extra check for each packet for the rare case of stats reset.

Quoted 7 lines · Ferruh Yigit

Quoted 7 lines

Quoted 2 lines

Not sure about introducing an extra check to datapath for possible error on stats zero. I am for dropping this patch, what do you think?

This is literally adding one instruction to the datapath. Not even an atomic instruction. There is no effect on the performance caused by this change.

Quoted 6 lines

Quoted 3 lines

Is that not better than the user (like me who experienced this) wondering why they cannot zero the counters even when there is no traffic?

Can we have something like, stop the forwarding, clear the stats and start forwarding back?

This is what is broken. The stats do not zero because you have the two worker threads who are constantly performing:

kni_stats[port_id].rx_packets += num;

and

kni_stats[port_id].tx_packets += nb_tx;

You know how read/inc/write races work, right?

These are not atomic increments, so the other thread zeroing these counters is _always_ going to race with these worker threads overwriting the counters with the old values.

With no traffic it's worse because the worker threads perform these increments even more often!

Quoted 3 lines

Yes effect is small but it is for each packet, it doesn't make sense to me to add extra check for each packet for the rare case of stats reset.

Its not rare at all! You cannot zero the statistics around 80% of the time on my machine when there is no traffic. It's trivial to reproduce this. Just run a little traffic, stop the traffic, zero the stats and check the stats.

If you cannot zero the statistics reliably under any circumstance then the statistics themselves are worthless and should be removed. It's better to have no stats than completely unreliable ones.

Quoted 3 lines

With no traffic it's worse because the worker threads perform these increments even more often!

Dear Dan,

Your implementation doesn't prevent the race when there is traffic, it can be

useful when there is no traffic, that is why my suggestion was in signal handler, stop worker threads, zero stats, start workers again.
But for traffic case this will cause packet lost so overall may not be desirable.

Quoted 7 lines

Quoted 2 lines

Yes effect is small but it is for each packet, it doesn't make sense to me to add extra check for each packet for the rare case of stats reset.

Its not rare at all! You cannot zero the statistics around 80% of the time on my machine when there is no traffic. It's trivial to reproduce this. Just run a little traffic, stop the traffic, zero the stats and check the stats.

I was trying to say, packet transfer is millions per second, so the waste is in that scale, but clear stats will be called in the scale of seconds/minutes? Or not at all perhaps for some cases?

Quoted 3 lines

If you cannot zero the statistics reliably under any circumstance then the statistics themselves are worthless and should be removed. It's better to have no stats than completely unreliable ones.

How much really you rely on sample application stats clear, and stats are still useful of course since you can use them without reset.

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

Hi All,

Attached is version 3 of a patchset to add a new API function to set the link status on kernel interfaces created with the KNI kernel module.

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

Quoted 14 lines

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

Quoted 8 lines

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

thanks
dan

Dan Gora (6):
kni: add API to set link status on kernel interface
kni: add link status test
kni: set default carrier state to 'off' examples/kni: monitor and update link status continually examples/kni: add log msgs to show and clear stats examples/kni: improve zeroing statistics

Quoted 41 lines · Dan Gora

Hi All,

Attached is version 3 of a patchset to add a new API function to set the link status on kernel interfaces created with the KNI kernel module.

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

Quoted 14 lines

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

Quoted 8 lines

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

thanks
dan

Quoted 7 lines

Dan Gora (6):
kni: add API to set link status on kernel interface
kni: add link status test
kni: set default carrier state to 'off' examples/kni: monitor and update link status continually examples/kni: add log msgs to show and clear stats examples/kni: improve zeroing statistics

Hi Dan,

We are a little away to integration deadline, it is good to clarify the status of the patchset.

There are a few change requests to this patchset:
1- 4/6, there is an open on adding command line option to control monitor/set link status.

2- Dropping 6/6, I guess you already agreed on this.

3- 1/6, to have or not the log message.

I would like to see the patchset in the release, what do you think about above actions?

Quoted 51 lines · Ferruh Yigit

Quoted 41 lines · Dan Gora

Hi All,

Attached is version 3 of a patchset to add a new API function to set the link status on kernel interfaces created with the KNI kernel module.

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

Quoted 14 lines

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

Quoted 8 lines

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

thanks
dan

Quoted 7 lines

Dan Gora (6):
kni: add API to set link status on kernel interface
kni: add link status test
kni: set default carrier state to 'off' examples/kni: monitor and update link status continually examples/kni: add log msgs to show and clear stats examples/kni: improve zeroing statistics

Quoted 8 lines

Hi Dan,

We are a little away to integration deadline, it is good to clarify the status of the patchset.

There are a few change requests to this patchset:
1- 4/6, there is an open on adding command line option to control monitor/set link status.

No, I don't have any plans to add a command line option to do this. Again, there is no reason for a command line option.

Quoted 1 line

2- Dropping 6/6, I guess you already agreed on this.

No, I showed you that that patch to fix zeroing the statistics would actually increase performance. You just never responded to that email.

Quoted 1 line

3- 1/6, to have or not the log message.

I included a proposal in my last email. Please take a look at that and respond.

Quoted 2 lines

I would like to see the patchset in the release, what do you think about above actions?

I'm incredibly frustrated with this whole process to be honest...

Quoted 1 line

I'm incredibly frustrated with this whole process to be honest...

Please don't be so, you are spending time/effort to improve an open source project which is great, thank you again.

Why don't we take this as incremental steps, first get agreed part in a patchset. 4/6 doesn't fit this but I can implement the command line part for you if you agree, what do you think?

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

Hi All,

Attached is version 4 of a patchset to add a new API function to set the link status on kernel interfaces created with the KNI kernel module.

v4
====
* Rework rte_kni_update_link to only take linkup/linkdown as parameter, return previous link state, and remove log messages.

* Update patch to set default carrier state to make default carrier state configurable by passing the 'carrier=[on|off]' option to the rte_kni kernel module. This is necessary in order to allow applications which use KNI as pure virtual interfaces without corresponding physical ethernet port to use the interfaces without having to set the carrier state to 'on' via rte_kni_update_link() or by writing to /sys/devices/virtual/net/<ifaceX>/carrier. Note that the default is 'off'.

* Add command line flag '-m' to examples/kni to continuously monitor and update the KNI interface link status according to the link status of the corresponding physical ethernet port.

Quoted 23 lines

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

Quoted 8 lines

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.

thanks
dan

Dan Gora (6):
kni: add API to set link status on kernel interface
kni: add link status test
kni: set default carrier state of interface examples/kni: monitor and update link status continually examples/kni: add log msgs to show and clear stats examples/kni: improve zeroing statistics

Add a new API function to KNI, rte_kni_update_link() to allow DPDK applications to update the link status for KNI network interfaces in the linux kernel.

Signed-off-by Dan Gora <[email protected]>

---
 lib/librte_kni/rte_kni.c           | 41 ++++++++++++++++++++++++++++++
 lib/librte_kni/rte_kni.h           | 20 +++++++++++++++
 lib/librte_kni/rte_kni_version.map |  6 +++++
 3 files changed, 67 insertions(+)

lib/librte_kni/rte_kni.c +21 -0

index b8edd40f4..c9726d4f8 100644
@@ -717,6 +717,47 @@ rte_kni_unregister_handlers(struct rte_kni *kni)
 
 	return 0;
 }
+
+int __rte_experimental
+rte_kni_update_link(struct rte_kni *kni, unsigned int linkup)
+{
+	char path[64];
+	char old_carrier[2];
+	const char *new_carrier;
+	int old_linkup;
+	int fd, ret;
+
+	if (kni == NULL)
+		return -1;
+
+	snprintf(path, sizeof(path), "/sys/devices/virtual/net/%s/carrier",
+		kni->name);
+
+	fd = open(path, O_RDWR);
+	if (fd == -1) {
+		RTE_LOG(ERR, KNI, "Failed to open file: %s.\n", path);
+		return -1;
+	}

KNI sample application calls this API each 100ms, so this keeps writing "carrier" file.

What do you think writing to "carrier" file only if status changed?

And it is possible to register to RTE_ETH_EVENT_INTR_LSC event in sample application instead of polling but not sure if all drivers supports it.

Or what do you think storing the link status in sample application and call this function only if link status changed?

kernel/linux/kni/kni_misc.c +5 -0

+static int __init
+kni_parse_carrier_state(void)
+{
+	if (!carrier)
+		dflt_carrier = 0;

v4
====
* Rework rte_kni_update_link to only take linkup/linkdown as parameter, return previous link state, and remove log messages.

* Update patch to set default carrier state to make default carrier state configurable by passing the 'carrier=[on|off]' option to the rte_kni kernel module. This is necessary in order to allow applications which use KNI as pure virtual interfaces without corresponding physical ethernet port to use the interfaces without having to set the carrier state to 'on' via rte_kni_update_link() or by writing to /sys/devices/virtual/net/<ifaceX>/carrier. Note that the default is 'off'.

* Add command line flag '-m' to examples/kni to continuously monitor and update the KNI interface link status according to the link status of the corresponding physical ethernet port.

v3
====
* Use separate function to test rte_kni_update_link() in 'test' app.

* Separate changes to 'test' app into separate patch to facilitate possible merge with https://patches.dpdk.org/patch/44730/

* Remove changes to set KNI interfaces to 'up' in example/kni

v2
====

* Fix bug where "Fixed" and "AutoNeg" were transposed in the link status log message.

* Add rte_kni_update_link() to rte_kni_version.map

* Add rte_kni_update_link() tests to kni_autotest

* Update examples/kni to continuously monitor link status and update the corresponding kernel interface with rte_kni_update_link().

* Minor improvements to examples/kni: Add log message showing how to show/zero stats. Improve zeroing statistics.

Note that checkpatches.sh compains about patch 1/5, but this appears to be a bug with check-symbol-change or something. If I move the fragment of the patch modifying rte_kni_version.map to the bottom of the patch file, it doesn't complain any more... I just don't really have time to investigate this right now.