Drop packets by default

[konne88]

It appears that BMv2 with v1model will, by default, forward packets to port 0.

I don't think v1model specifies what to do here (at least I couldn't find it), but PSA drops packets by default instead:

struct psa_ingress_output_metadata_t {
  // The comment after each field specifies its initial value when the
  // Ingress control block begins executing.
  ClassOfService_t         class_of_service; // 0
  bool                     clone;            // false
  CloneSessionId_t         clone_session_id; // initial value is undefined
  bool                     drop;             // **true**
  bool                     resubmit;         // false
  MulticastGroup_t         multicast_group;  // 0
  PortId_t                 egress_port;      // initial value is undefined
}

Could we change BMv2 to drop packets by default for v1model as well?

[jafingerhut]

The C++ code change in this repository is pretty trivial, I believe, e.g. assign an initial value of the DROP_PORT value to stdmeta.egress_spec before ingress code begins execution.

Perhaps the more difficult question to answer is who the set of people who care most about v1model architecture behavior is. You obviously care, or you would not be asking. The other main user today I can think of is the p4c compiler test suite, which I suspect would need some updates if a change like you describe were made.

Other possibilities, just brainstorming, with various levels of pain for different v1model users:

• a command line option for simple_switch / simple_switch_grpc that enables the different behavior you ask for, but by default continues with current behavior.
• Create a new architecture that is much like v1model, but with some changes, e.g. 'v2model'.

[konne88]

Thank Andy for the feedback.

This is not a huge problem, to get around this, we mark_to_drop all packets at the beginning of the ingress pipeline. I was just wondering if it's easy to fix, since dropping does seem like the more intuitive default behavior. Creating a new architecture certainly seems overkill for this problem :)

What exactly is the concern around p4c? Does p4c use BMv2? Is the worry that right now the p4c compiler programs hardware to use port 0 by default, but it would have to change to drop instead?

Does the P4C compiler user BMv2?

[jafingerhut]

There are a few hundred p4c test programs that run simple_switch_grpc processes to emulate the programs and pass packets, on every create/change of a PR on the p4lang/p4c repository.

Most of them probably do not depend upon the current v1model behavior -- implementing the change you ask about and trying to run all p4c tests would tell us how many, and fixing them if they broke should be straightforward in most cases.

[konne88]

Are you saying we could just change the BMv2 behavior to drop by default and we would automatically be told what we broke (if anything)? If that's the case, we should probably just try and see what happens.

[jafingerhut]

It would be less disruptive for p4c developers if the bmv2 change was made on someone's local machine, and the p4c tests run on that local machine. If we merge it into bmv2 repo, within hours it will potentially break p4c automated tests and prevent merging of PRs on the p4lang/p4c repository, which if we did that with full foreknowledge of doing that, they probably would greatly prefer the 'test it on a local machine first' approach. :-)

[antoninbas]

Even if the p4c test suite does not break, changing this default behavior may not be the right thing to do since this is unspecified in v1model. Other things (tutorials, third party programs, ...) may depend on the behavior. I like Andy's idea of a runtime flag to change that behavior when starting a bmv2 switch process.

[jafingerhut]

With a change to simple_switch_grpc so that stdmeta.egress_spec was initialized to drop_port, about 140 of the p4c tests fail.

[jafingerhut]

This PR does not seem to work as I was hoping yet, but it is probably at least half way to the changes desired to add a command line option to enable this non-default behavior: https://github.com/p4lang/behavioral-model/pull/993

[github-actions[bot]]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days