Add check for validating ports

[rohit-mp]

Added a simple check in onvm/go.sh to validate the given port mask.

Closes #139

Summary:

Usage:

This PR includes
Resolves issues	👍	#139
Breaking API changes
Internal API changes
Usability improvements
Bug fixes	👍
New functionality
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

• Dependencies: None

TODO before merging :

• [x] PR is ready for review

Test Plan:

Review:

@kevindweb

(optional) Subscribers: << @-mention people who probably care about these changes >>

[onvm]

In response to PR creation

CI Message

Your results will arrive shortly

[rohit-mp]

The script stops the user only when they specify a port value which is greater than the number of NICs bound. Additionally, gives a warning when the port value of 0 is used. I tested this against 0, 1, 2 port values.

Does this look good? Let me know if any changes are required.

[rohit-mp]

I seem to have incorrectly based my branch off of master instead of develop. I'll create another PR if that's an issue.

[kevindweb]

Thanks for making this so quickly. ONVM_NIC_PCI is not always filled. In scripts/setup_environment.sh, we check if that variable is empty, and request user response, but we don't actually set the variable. There's also the case that I don't set the NIC in setup, but I do it manually while testing through DPDK (which I've done many times). In those cases as well, we can't ensure the user has them set just through that variable.

[rohit-mp]

Ah forgot about that. I could modify it to check through all network devices instead of just those in the variable but that doesn't seem elegant. Do you have anything in mind?

[kevindweb]

Here is a command I think would work

./dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q'

This will only output the interfaces bound to dpdk, and stops outputting when we get to the kernel drivers. We can just grep to find a specific device, or count how many devices are bound.

[rohit-mp]

That's neat. Modifying the same to just count the number of times 'drv' appears would give us our required output.

./dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q' | grep -c "drv"

This should now give the number of network devices using dpdk compatible driver and can be directly compared with the portmask given. Does that sound good?

[rohit-mp]

Sorry for the delay. I've updated the checking condition. I hope the messages displayed are fine.

[kevindweb]

Thanks for updating. dpdk_drivers is probably unnecessary now. Can you make sure your tabs/spaces are correct (lines 64 and 65 specifically)? Also, we are trying to use [[ over [ single brace conditions in the future.

[rohit-mp]

Thanks for pointing that out. I'll remove dpdk_drivers. I'm not sure why the tabs were misaligned (looks proper in vim). Replaced tabs by spaces now.

Should I update [ to [[ in other parts of the script as well?

[kevindweb]

I would leave other parts of the script. It's not horrible, we just want to do it for the future updates, and to reduce the amount of testing you should just update your code if you can.

[kevindweb]

Looks good, I will have to update CI before we test this, because CI runs go.sh without the correct number of ports I think.

[rohit-mp]

Okay. Thanks for immediate responses!

[kevindweb]

Let's try this @onvm

[onvm]

Let's try this @onvm

CI Message

Your results will arrive shortly

[rohit-mp]

Thanks!

[kevindweb]

Ok well rip 👎, we need to go back to the drawing board. Your code works, but it doesn't solve the problem. When you don't bind any NIC (for example if the server doesn't have any) and we try to run with ./go.sh 0,1,2,3 0 0xF0 -s stdout and speed_tester with ./go.sh 1 -d 1. We still get the original issue of:

Invalid port_id=0
Invalid port_id=0
Invalid port_id=0
Invalid port_id=0
Invalid port_id=0
Invalid port_id=0

In this situation, I have no NICs bound, and the go script passes because I don't use any ports. Any ideas?

[rohit-mp]

Since we can't stop the user from running with no NICs bound, I guess we can't proceed further with these checks. I'll try to look into what's causing that output.

[twood02]

The root issue for speed tester is that https://github.com/sdnfv/openNetVM/blob/master/examples/speed_tester/speed_tester.c#L381 we call rte_eth_macaddr_get(0,...) on port 0, but there may not be a port 0. We should add a check in speed tester to be sure there is at least one port, and if not, we can use a default MAC address.

@rohit-mp -- can you update your PR to add a condition in speed tester to check that there are ports?

[rohit-mp]

Sorry @twood02, missed this notification before commenting on the issue.

The function rte_eth_dev_is_valid_port verifies if the port is valid or not. Does this change look good?

if (rte_eth_dev_is_valid_port(0)) {
        rte_eth_macaddr_get(0, &ehdr->s_addr);
} else {
        ehrd->s_addr = fake_mac_address;
}

[twood02]

exactly, that will be perfect. You can make up a fake address, but pick from one of the locally administered ranges: https://honeywellaidc.force.com/supportppr/s/article/Locally-Administered-MAC-addresses

[rohit-mp]

After going through load_balancer.c, I see another viable solution:

extern struct port_info *ports;

if (ports->num_ports > 0) {
    rte_eth_macaddr_get(0, &ehdr->s_addr);
} else {
    ehdr->s_addr = fake_mac_addr;
}

I've tested both and both seem to work. Which do I go ahead with?

[twood02]

Let's use rte_eth_dev_is_valid_port

[kevindweb]

Tested scaling, speed_tester, and load_generator successfully so far. Not sure why we need

extern struct port_info *ports;

I just tested speed_tester without that and it worked fine.

The load balancer is harder to test because inherently it needs 2 NICs bound. The README states this as such:

This NF requires 2 DPDK interfaces to work, both can be setup using the mTCP submodule iface setup

The load balancer drops the packets, thus not entering the pkt_handler if the packets don't have the correct source/destination address.

[dennisafa]

I'm questioning whether or not its a good idea to always fake the mac addr. I think that in some cases it's necessary, like with speed_tester since we are generating fake packets. But for the load_balancer and maybe future NF's, we might want to be sure we're using a real port. I'm thinking we should separate out the functionality for obtaining the mac addr and faking it. thoughts on this? @kevindweb @twood02

[kevindweb]

I see your point, but I was also thinking that load_balancer can't even run without a port, so it may be better to just stop processing in that NF if rte_eth_dev_is_valid_port returns false, right? This logic could be applied to any new NF that requires ports that cannot be faked.

[dennisafa]

I see your point, but I was also thinking that load_balancer can't even run without a port, so it may be better to just stop processing in that NF if rte_eth_dev_is_valid_port returns false, right? This logic could be applied to any new NF that requires ports that cannot be faked.

Since it depends on the NF functionality, it should be the NF devs choice to stop running the NF if they cannot obtain a valid port, or call a separate function to retrieve a faked port. That's why I think that we should have two functions, onvm_get_mac_addr and something like onvm_get_fake_mac_addr if necessary.

[kevindweb]

@dennisafa Sounds like a good plan to me. @rohit-mp would that work for you?

[rohit-mp]

Sounds good. Just one doubt, get_fake would just return a fake address or try to check if a port is available? And to confirm, load_balancer and load_generator are the only two NFs that require a port to be bound right?

[dennisafa]

Sounds good. Just one doubt, get_fake would just return a fake address or try to check if a port is available? And to confirm, load_balancer and load_generator are the only two NFs that require a port to be bound right?

Lets have fake address just return a fake mac address with the intention of being used when real ports are not required in an NF. I think it would be useful to have. I believe that those two NF's are the only ones, yes. It may be worth testing them with/without fake mac's to see if that holds true.

[rohit-mp]

Okay, we could do that.

I was also wondering if maybe we could use ehdr->ether_type to denote if the src/dst mac_addr is real/fake. I'm not sure for what it's being used right now but that way, we could work with a single macro and set the bit accordingly on updating the address. Or we could have a function instead of a macro with appropriate return values for real/fake addresses.

[dennisafa]

Lets make it a real function so that we can interpret return values

On Fri, Apr 17, 2020 at 3:39 PM Rohit M P notifications@github.com wrote:

Okay, we could do that.

I was also wondering if maybe we could use ehdr->ether_type to denote if the src/dst mac_addr is real/fake. I'm not sure for what it's being used right now but that way, we could work with a single macro and set the bit accordingly on updating the address. Or we could have a function instead of a macro with appropriate return values for real/fake addresses.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/sdnfv/openNetVM/pull/205#issuecomment-615429766, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH3EIZS2JW7SS653JS5BWCTRNCV6XANCNFSM4MEPVI6A .

[rohit-mp]

Does this look good?

[rohit-mp]

I've also updated the function name to make it more readable. I'd used onvm_macaddr_get before to make it similar to rte_eth_macaddr_get.

[kevindweb]

Hate to be that guy again but can you remove the speed_tester port extern line? I'll test and approve once you do.

[rohit-mp]

Nothing to say for missing that a third time 😅

[rohit-mp]

Thanks for the careful reviews!

[kevindweb]

@onvm sanity check?

[onvm]

@onvm sanity check?

CI Message

Your results will arrive shortly