Thanks for the comments. Replies below. -Matias
> -----Original Message----- > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of EXT Maxim > Uvarov > Sent: Wednesday, February 17, 2016 4:02 PM > To: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH 2/2] linux-generic: pktio: add support for > netmap > VALE and pipe virtual ports > > Ok, in general it works, some comment bellow. > > On 02/16/16 16:21, Matias Elo wrote: > > As well as connections to a physical network interface the netmap API > > also supports connections to two different types for virtual ports, VALE > > switch ports and netmap pipe ports. VALE is a virtual switch implemented > > in the netmap kernel module, it works as a learning bridge and is designed > > to be used for connections between virtual machines. netmap pipes connect > > two virtual netmap ports with a crossover connection, they can be used in > > a setup where a master process works as a dispatcher towards slave > > processes. > > > > Virtual netmap ports are given a dummy MAC address from locally > > administered MAC address range (02:00:00:00:00:XX). > > > > Signed-off-by: Matias Elo <matias....@nokia.com> > > Suggested-by: Stuart Haslam <stuart.has...@linaro.org> > > --- > > platform/linux-generic/include/odp_packet_netmap.h | 1 + > > platform/linux-generic/pktio/netmap.c | 43 > > ++++++++++++++++++---- > > 2 files changed, 37 insertions(+), 7 deletions(-) > > > > diff --git a/platform/linux-generic/include/odp_packet_netmap.h > b/platform/linux-generic/include/odp_packet_netmap.h > > index 6338c36..ebb0f8d 100644 > > --- a/platform/linux-generic/include/odp_packet_netmap.h > > +++ b/platform/linux-generic/include/odp_packet_netmap.h > > @@ -45,6 +45,7 @@ typedef struct { > > unsigned char if_mac[ETH_ALEN]; /**< eth mac address */ > > char nm_name[IF_NAMESIZE + 7]; /**< netmap:<ifname> */ > > char if_name[IF_NAMESIZE]; /**< interface name used in ioctl */ > > + odp_bool_t is_virtual; /**< nm virtual port (VALE/pipe) */ > > odp_pktio_capability_t capa; /**< interface capabilities */ > > unsigned cur_rx_queue; /**< current pktin queue */ > > uint32_t num_rx_rings; /**< number of nm rx rings */ > > diff --git a/platform/linux-generic/pktio/netmap.c b/platform/linux- > generic/pktio/netmap.c > > index d869c22..d45d591 100644 > > --- a/platform/linux-generic/pktio/netmap.c > > +++ b/platform/linux-generic/pktio/netmap.c > > @@ -210,6 +210,9 @@ static int netmap_close(pktio_entry_t *pktio_entry) > > > > static int netmap_link_status(pktio_entry_t *pktio_entry) > > { > > + if (pktio_entry->s.pkt_nm.is_virtual) > > + return 1; > > + > > return link_status_fd(pktio_entry->s.pkt_nm.sockfd, > > pktio_entry->s.pkt_nm.if_name); > > } > > @@ -278,9 +281,13 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, > pktio_entry_t *pktio_entry, > > odp_buffer_pool_tailroom(pool); > > > > /* allow interface to be opened with or without the 'netmap:' prefix */ > > - if (strncmp(netdev, "netmap:", 7) == 0) > > - netdev += 7; > > prefix = "netmap:"; > > + if (strncmp(netdev, "netmap:", 7) == 0) { > > + netdev += 7; > > + } else if (strncmp(netdev, "vale", 4) == 0) { > > + pkt_nm->is_virtual = 1; > > + prefix = ""; > > + } > > > > snprintf(pkt_nm->nm_name, sizeof(pkt_nm->nm_name), "%s%s", > prefix, > > netdev); > > @@ -317,6 +324,24 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, > pktio_entry_t *pktio_entry, > > buf_size = ring->nr_buf_size; > > nm_close(desc); > > > > + for (i = 0; i < PKTIO_MAX_QUEUES; i++) { > > + odp_ticketlock_init(&pkt_nm->rx_desc_ring[i].s.lock); > > + odp_ticketlock_init(&pkt_nm->tx_desc_ring[i].s.lock); > > + } > > + > > + if (pkt_nm->is_virtual) { > > + static unsigned mac; > > + > > + pkt_nm->capa.max_input_queues = 1; > > + pkt_nm->mtu = buf_size; > > + pktio_entry->s.stats_type = STATS_UNSUPPORTED; > > + /* Set MAC address for virtual interface */ > > + pkt_nm->if_mac[0] = 0x2; > > + pkt_nm->if_mac[5] = ++mac; > > + > If early return here, than it's needed to disable statistics if not > implemented, to not fail on pktio test. > > pktio_entry->s.stats_type = STATS_UNSUPPORTED; > 'pktio_entry->s.stats_type = STATS_UNSUPPORTED;' is already set in the if statement above. > and also make it for pipe mode, where is_virtual is not set. As far as I know also netmap pipe interface names have to start with 'vale', so 'is_virtual ' should be set also for them. > > > + return 0; > > + } > > + > > sockfd = socket(AF_INET, SOCK_DGRAM, 0); > > if (sockfd == -1) { > > ODP_ERR("Cannot get device control socket\n"); > > @@ -350,11 +375,6 @@ static int netmap_open(odp_pktio_t id ODP_UNUSED, > pktio_entry_t *pktio_entry, > > if (err) > > goto error; > > > > - for (i = 0; i < PKTIO_MAX_QUEUES; i++) { > > - odp_ticketlock_init(&pkt_nm->rx_desc_ring[i].s.lock); > > - odp_ticketlock_init(&pkt_nm->tx_desc_ring[i].s.lock); > > - } > > - > > /* netmap uses only ethtool to get statistics counters */ > > err = ethtool_stats_get_fd(pktio_entry->s.pkt_nm.sockfd, > > pkt_nm->if_name, &cur_stats); > > @@ -430,6 +450,7 @@ static int netmap_start(pktio_entry_t *pktio_entry) > > pktio_entry->s.num_out_queue, > > pktio_entry->s.num_out_queue); > > > > + memset(&base_desc, 0, sizeof(struct nm_desc)); > > base_desc.self = &base_desc; > > base_desc.mem = NULL; > > memcpy(base_desc.req.nr_name, pkt_nm->if_name, sizeof(pkt_nm- > >if_name)); > > @@ -774,12 +795,20 @@ static int netmap_mtu_get(pktio_entry_t > *pktio_entry) > > static int netmap_promisc_mode_set(pktio_entry_t *pktio_entry, > > odp_bool_t enable) > > { > > + if (pktio_entry->s.pkt_nm.is_virtual) { > > + __odp_errno = ENOTSUP; > > + return -1; > > + } > > + > > In test case we have: > > ret = odp_pktio_promisc_mode_set(pktio, 1); > CU_ASSERT(0 == ret); > > After returning 0 validation almost tests passes. I.e. only statistics > does not work. > > > > return promisc_mode_set_fd(pktio_entry->s.pkt_nm.sockfd, > > pktio_entry->s.pkt_nm.if_name, enable); > > } > > > > static int netmap_promisc_mode_get(pktio_entry_t *pktio_entry) > > { > > + if (pktio_entry->s.pkt_nm.is_virtual) > > + return 0; > > + > Same issue here. > > I think that promisc mode does not make any sense for pktio like loop, > tap and other virtual things. I agree. I'll add a new bit field struct to odp_pktio_capability_t for checking supported set operations. For now it will only include promisc mode, but other set operations can be added later if needed. > It might be better to make promisc function optional and use conditional > test in cunit. > > > return promisc_mode_get_fd(pktio_entry->s.pkt_nm.sockfd, > > pktio_entry->s.pkt_nm.if_name); > > } I'd rather add the aforementioned new bit mask and modify the tests. > > You can apply on top: > linux-generic: validation: add netmap test > it applies cleanly, but you need change: > if [ "$(lsmod | grep netmap | grep veth)" = "" ]; then > to > if [ "$(lsmod | grep netmap)" = "" ]; then > > After that under root you will be able to run pktio test with: > ./platform/linux-generic/test/pktio/pktio_run_netmap > Thanks, I'll use this to test the next version. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
