Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

click/etherswitch: Forward traffic to a set of ports #334

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SDarayan
Copy link

The default behavior of etherswitch is to forward traffic
to all ports except the source when broadcast or to the
destination port found in lookup of etherswitch's mac table.

This commit expands the functionality of etherswitch to
send packet(s) out port(s) iff the port to forward traffic
is enabled. Thus, etherswitch can selectively forward traffic
to particular ports, a feature proving useful with broadcast
traffic so as to avoid traffic loops between click and other
external hardware (e.g. switch).

Ports can selectively be removed and the port forwarding map
can be reset via write handlers "remove_port_forwarding" and
"reset_port_forwarding".

The arguments to "remove_port_forwarding" write handler is a
list of numbers with the first one being a source port and
the others being ports to remove.

For example, "1 2 3 4, 2 3 4" means port 1 will not forward
traffic to ports 2, 3, and 4 -- in addition port 2 will not
forward traffic to ports 3 and 4.

No arguments are necessary for "reset_port_forwarding" write
handler.

Also, the port forwarding map can be viewed via read handler
"port_forwarding".

@SDarayan
Copy link
Author

@tbarbette Could you take a look at this one and comment or commit to mainline?

.read("TIMEOUT", SecondsArg(), _timeout)
.complete();
.complete() < 0)
return errh->error("bad timeout");
Copy link
Collaborator

@tbarbette tbarbette May 21, 2017

Choose a reason for hiding this comment

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

Just return -1. The error could be that there is an unknown argument, not bad timeout. In both ease read and complete will already add the error msg.

@tbarbette
Copy link
Collaborator

Apart from the small comment it looks good to me. I just wonder about the performance impact? It is a fast-path element and that may effect a lot of peoples. This adds at least two indirections per packets.
I can run a little perf test in a week to check that. Maybe it would be better to make this an AdvancedEtherSwitch if it is too big?

I don't merge non-DPDK related commits myself. @kohler will probably have thoughts about this anyway.

@ahenning
Copy link

I like the concept of this element and find the functionality useful. But agree that it would be preferred to have it as a separate element.

Also, I had some issues compiling in userlevel due to some errors related to bitvector functions. It was related to the weight if I recall correctly.

Another little nitpick, it would be nice to have the usage example in the header vs git comment.

@tbarbette
Copy link
Collaborator

tbarbette commented Jul 4, 2017

I tried to generate multiple L2 flows (1-256) to evaluate the performance impact of this element but I could not see any clear impact.

I guess this make a very slow path (after all broadcasts are not the usual) just a little bit heavier, so at the end, it is not that much of a problem.

@SDarayan I've re-imported the dependencies in tbarbette/click in https://github.com/tbarbette/click/tree/fixdeps , you would need to include those

I agree with the usage in the header. I can see where the practice come from :p

cliff and others added 4 commits July 4, 2017 10:30
Signed-off-by: Patrick Verkaik <[email protected]>
Returns the number of bits equal to one
Bitvector::parse(str, min_val, max_val, offset = 0)
parses a comma separated list of values and dashed value ranges into
the bitvector.

'min_val' and 'max_val' are the minimum and maximum values allowed
during parsing.  'offset' pads out extra false bits at the beginning
of the bitvector before the bit corresponding to 'min_val'.  Setting
'offset' to 1 can be used to create one based indexing.

Examples:
parse("1,3,5-8", 1, 10) -> 1010111100
parse("1,3", 1, 3, 3) -> 000101
parse("-5,-2--1", -5, 0) -> 100110

Bitvector::unparse(read_offset = 0, output_offset = 0)
outputs a string representing the set bits in the bitvector.

11010.unparse() -> "0-1,3"

A positive 'read_offset' skips the specified number of bits and
decreases the output values the same number:
11010.unparse(1) -> "0,2"

A negative 'read_offset' pads the output with the specified number of
unset bits (this is the same as just increasing output_offset the same amount):
11010.unparse(-1) -> "1-2,4"

'output_offset' increases or decreases the output values:
11010.unparse(0, 1) -> "1-2,4"
11010.unparse(0, -1) -> "-1-0,2"

Reviewed-by: [email protected]
Signed-off-by: Patrick Verkaik <[email protected]>
The default behavior of etherswitch is to forward traffic
to all ports except the source when broadcast or to the
destination port found in lookup of etherswitch's mac table.

This commit expands the functionality of etherswitch to
send packet(s) out port(s) iff the port to forward traffic
is enabled. Thus, etherswitch can selectively forward traffic
to particular ports, a feature proving useful with broadcast
traffic so as to avoid traffic loops between click and other
external hardware (e.g. switch).

Ports can selectively be removed and the port forwarding map
can be reset via write handlers "remove_port_forwarding" and
"reset_port_forwarding".

The arguments to "remove_port_forwarding" write handler is a
list of numbers with the first one being a source port and
the others being ports to remove.

For example, "1 2 3 4, 2 3 4" means port 1 will not forward
traffic to ports 2, 3, and 4 -- in addition port 2 will not
forward traffic to ports 3 and 4.

No arguments are necessary for "reset_port_forwarding" write
handler.

Also, the port forwarding map can be viewed via read handler
"port_forwarding".
@ahenning
Copy link

ahenning commented Jul 5, 2017

@tbarbette @SDarayan does this not reintroduce issue #327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants