Hi Ilya,
Please find the replies inline.
> >> #include "dpif-netdev-private-dpcls.h"
> >> +#include "dpif-netdev-private-dpif.h"
> >> +#include "dpif-netdev-private-extract.h"
>
> "private" dpif-netdev headers should not be included in non dpif-netdev
> modules.
>
There are many structures which reside in these headers which we need because
of the
API refactors for example : struct netdev_flow_key which resides in the
private-dpcls.h
which mfex func API uses.
> >> #include "openflow/openflow.h"
> >> #include "packets.h"
> >> #include "odp-util.h"
> >> @@ -757,7 +759,7 @@ dump_invalid_packet(struct dp_packet *packet,
> >> const char *reason)
> >> * of interest for the flow, otherwise UINT16_MAX.
> >> */
> >> void
> >> -miniflow_extract(struct dp_packet *packet, struct netdev_flow_key
> >> *key)
> >> +miniflow_extract_(struct dp_packet *packet, struct netdev_flow_key
> >> +*key)
> >> {
> >> /* Add code to this function (or its callees) to extract new fields.
> >> */
> >> BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); @@ -1112,6 +1114,36
> @@
> >> miniflow_extract(struct dp_packet *packet, struct netdev_flow_key *key)
> >> key->mf.map = mf.map;
> >> }
> >>
> >> +void
> >> +miniflow_extract(struct dp_packet *packet, struct netdev_flow_key
> >> +*key) { #ifdef MFEX_AUTOVALIDATOR_DEFAULT
> >> + static struct ovsthread_once once_enable =
> >> OVSTHREAD_ONCE_INITIALIZER;
> >> + if (ovsthread_once_start(&once_enable)) {
> >> + dpif_miniflow_extract_init();
> >> + ovsthread_once_done(&once_enable);
> >> + }
> >> + struct dp_packet_batch packets;
> >> + const struct pkt_metadata *md = &packet->md;
> >> + dp_packet_batch_init(&packets);
> >> + dp_packet_batch_add(&packets, packet);
> >> + const uint32_t recirc_depth = *recirc_depth_get();
> >> +
> >> + /* Currently AVX512 DPIF dont support recirculation
> >> + * Once the support will be added the condition would
> >> + * be removed.
> >> + */
> >> + if (recirc_depth) {
> >> + miniflow_extract_(packet, key);
> >> + } else {
> >> + dpif_miniflow_extract_autovalidator(&packets, key, 1,
> >> + odp_to_u32(md->in_port.odp_port),
> >> NULL);
> >> + }
> >> +#else
> >> + miniflow_extract_(packet, key);
> >> +#endif
>
> This doesn't seem right. First of all, it will be impossible to disable the
> autovalidator if it was chosen in compile time as a default option.
> There will be also significant performance impact.
>
> And, in general, that is not what I was asking for.
>
> The idea was to move the implementation selector here, so any part of the
> code, including AVX512 dpif implementation, can use same plain
> [mini]flow_extract() to parse the packet and have actual implementation
> chosen based on the current settings/priorities.
>
We tried the following approach as suggested above, but we observed the
following problems mentioned below in details:
1. Per packet based scalar mfex API vs Per batch based AVX512 APIs.
We need to create a packet batch first, since that’s what the AVX512 APIs
accept then process the packet then clone back the packet to the original
packet.
struct dp_packet_batch batch_;
dp_packet_batch_init(&batch_);
dp_packet_batch_add(&batch_, packet);
miniflow_extract(&batch_, key, 1,
odp_to_u32(port_no), pmd);
packet = dp_packet_clone(batch_.packets[0]);
2. The mfex function pointer for the current implementation lives in the PMD
struct.
-> Not all places where miniflow_extract or flow_extract has
access to pmd struct.
-> To fix the problem we need to either:
1. Pass in the PMD struct everywhere.
2. Fetch the mfex implementation every time.
> Best regards, Ilya Maximets.
>
Regards
Amber
<snip>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
