p4lang / behavioral-model

The reference P4 software switch
Apache License 2.0
532 stars 327 forks source link

clone_preserving_field_list cannot clone to port number > 255 #1162

Closed tiantianz closed 1 year ago

tiantianz commented 1 year ago

@smolkaj We implemented a P4 program in which we use clone_preserving_field_list with CloneType.I2E to clone packets. But we found that it doesn't support port number > 255. If we create a clone session and configure egress_port to any number <= 255, it works correctly. But if we create a clone session and configure egress_port to any number > 255, it doesn't work and the log always says "Replicating packet on port 255".

Is this a bug of BMv2?

Thank you!

jafingerhut commented 1 year ago

I am not sure, but there appear to be some places in the control plane code for behavioral-model when you configure multicast groups that use uint8 for ports, rather than some larger type. I am not familiar with that part of the code much, but it sounds like a limitation that is part of the implementation somewhere in there, and no one has tried to use port numbers larger than this before for this purpose.

"One man's bug is another man's undocumented limitation." ?

I'm sure that this limit could be increased with the proper code changes, but I don't have any time estimate on how long it would take, as I'm not sure what the scope of those changes is.

How large of a numeric range of port numbers are you hoping to use?

tiantianz commented 1 year ago

Thanks!

We are trying to set the CPU port to 511 and use clone_preserving_field_list to clone packets to the CPU port.

jafingerhut commented 1 year ago

For your information, I have no plans to make changes to behavioral-model code to enhance this limitation any time soon. I simply took a quick look to see if it was a one-line change somewhere, but it is not clear to me that it is a one-line change.

I would recommend persuading someone in your organization to dig into the behavioral-model code a bit and see if they can code up the changes required, if you want this to be enhanced.

antoninbas commented 1 year ago

@jafingerhut is right this code and the comment there is a good reference: https://github.com/p4lang/behavioral-model/blob/2bb8a93454c6a3e3c997c33573937d5266290ad0/PI/src/pi_mc_imp.cpp#L70-L82 The multicast engine uses a static bitset of size PORT_MAP_SIZE (256) to represent ports, and was not updated when we started adding support for arbitrary 32 bit port values. While supporting arbitrary port value (i.e., any uint32) would be a bit of work, a quick workaround if you want things to work with port 511 would be to update PORT_MAP_SIZE to 512: https://github.com/p4lang/behavioral-model/blob/27c235944492ef55ba061fcf658b4d8102d53bd8/include/bm/bm_sim/simple_pre.h#L85 Anything up to 1024 or even 4096 would be fine by me.

smolkaj commented 1 year ago

Thanks for the info @antoninbas. I like the proposed workaround, @tiantianz or I will be sending out a PR to implement it.