Thanks for the review. Comments inline.
-Matias
+/**
+ * Compute packet flow index
+ *
+ * @param hdr Packet headers
+ *
+ * @return Flow index
+ */
+static inline uint64_t calc_flow_idx(packet_hdr_t *hdr)
+{
+ ipv4_tuple5_t tuple;
+ uint64_t idx;
+
+ tuple.dst_ip = odp_be_to_cpu_32(hdr->ipv4->dst_addr);
+ tuple.src_ip = odp_be_to_cpu_32(hdr->ipv4->src_addr);
+ tuple.proto = hdr->ipv4->proto;
+ tuple.src_port = odp_be_to_cpu_16(hdr->udp->src_port);
+ tuple.dst_port = odp_be_to_cpu_16(hdr->udp->dst_port);
+ tuple.pad0 = 0;
+ tuple.pad1 = 0;
+ idx = calc_ipv4_5tuple_hash(&tuple);
+
+ return idx % gbl_args->appl.num_flows;
+}
can all that static inlines go to .h file? I think splitting code by files
makes it more easy for understanding.
In my opinion they shouldn’t. These inline function are only used by this
application, so there is no real benefit
gained from separating them into a header file. It’s also more simple to check
the function implementations from
within the same file.
+
+/**
+ * Create a pktio handle and associate with input queues
+ *
+ * @param dev Name of device to open
+ * @param index Pktio index
+ * @param pool Pool to associate with device for packet RX/TX
+ *
@param num_rx
@param num_tx
Will fix.
+ * @retval 0 on success
+ * @retval -1 on failure
+ */
+static int create_pktio(const char *dev, int idx, int num_rx, int num_tx,
+ odp_pool_t pool)
+{
+ odp_pktio_t pktio;
+ odp_pktio_param_t pktio_param;
+ odp_pktio_capability_t capa;
+ odp_pktin_queue_param_t pktin_param;
+ odp_pktout_queue_param_t pktout_param;
+ odp_pktio_op_mode_t mode_rx;
+ odp_pktio_op_mode_t mode_tx;
+ int i;
+
+ odp_pktio_param_init(&pktio_param);
+
+ pktio_param.in_mode = ODP_PKTIN_MODE_SCHED;
+
+ pktio = odp_pktio_open(dev, pool, &pktio_param);
+ if (pktio == ODP_PKTIO_INVALID) {
+ LOG_ERR("Error: failed to open %s\n", dev);
+ return -1;
+ }
+
+ printf("Created pktio %" PRIu64 " (%s)\n",
+ odp_pktio_to_u64(pktio), dev);
+
+ if (odp_pktio_capability(pktio, &capa)) {
+ LOG_ERR("Error: capability query failed %s\n", dev);
Resource leak on error. Needed to close pktio.
Will fix.
+/**
+ * Initialize port forwarding table
+ */
+static void init_forwarding_tbl(void)
+{
+ int rx_idx;
+
+ for (rx_idx = 0; rx_idx < gbl_args->appl.if_count; rx_idx++)
+ gbl_args->dst_port[rx_idx] = find_dest_port(rx_idx);
+}
+
+/**
+ * Prinf usage information
+ */
+static void usage(char *progname)
+{
+ printf("\n"
+ "OpenDataPlane ordered pktio application.\n"
+ "\n"
+ "Usage: %s OPTIONS\n"
+ " E.g. %s -i eth0,eth1 -t 1\n"
if -t is optional that it can be simple -i eth0,eth1,eth2,eth3
Will fix.
+static void parse_args(int argc, char *argv[], appl_args_t *appl_args)
+{
+ int opt;
+ int long_index;
+ char *token;
+ char *addr_str;
+ size_t len;
+ int i;
+ static const struct option longopts[] = {
+ {"count", required_argument, NULL, 'c'},
+ {"time", required_argument, NULL, 't'},
+ {"accuracy", required_argument, NULL, 'a'},
+ {"interface", required_argument, NULL, 'i'},
+ {"mode", required_argument, NULL, 'm'},
+ {"out_mode", required_argument, NULL, 'o'},
not documented out_mode
Will fix.
+
+ /* Reserve memory for args from shared mem */
+ shm = odp_shm_reserve("shm_args", sizeof(args_t),
+ ODP_CACHE_LINE_SIZE, 0);
shm != ODP_SHM_INVALID
Will fix.
+ gbl_args = odp_shm_addr(shm);
+
+ if (gbl_args == NULL) {
+ LOG_ERR("Error: shared mem alloc failed.\n");
+ exit(EXIT_FAILURE);
+ }
+ gbl_args_init(gbl_args);
+
+ /* Parse and store the application arguments */
+ parse_args(argc, argv, &gbl_args->appl);
+
+ if (gbl_args->appl.in_mode == SCHED_ORDERED) {
+ /* At least one ordered lock required */
+ odp_queue_capability(&capa);
+ if (capa.max_ordered_locks < 1) {
+ LOG_ERR("Error: Ordered locks not available.\n");
+ exit(EXIT_FAILURE);
+ }
+ }
+ /* Print both system and application information */
+ print_info(NO_PATH(argv[0]), &gbl_args->appl);
+
+ /* Default to system CPU count unless user specified */
+ num_workers = MAX_WORKERS;
MAX_WORKERS is not needed you can do
num_workers = odp_cpumask_default_worker(&cpumask, 0);
and allocate thread_tbl here.
The application allocates per worker memory from shm (args_t: stats, thread),
so the MAX_WORKERS
define is still needed.
+ if (gbl_args->appl.cpu_count)
+ num_workers = gbl_args->appl.cpu_count;
+
+ /* Get default worker cpumask */
+ num_workers = odp_cpumask_default_worker(&cpumask, num_workers);
+ (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr));
+
+ gbl_args->appl.num_workers = num_workers;
+
+ for (i = 0; i < num_workers; i++)
+ gbl_args->thread[i].thr_idx = i;
spaces can be removed. And I do not see where thr_idx is used. Can be removed?
Will fix.
