RE: [PATCH v5 7/7] app/testpmd: add Host Shaper command

[Spike Du <spiked@nvidia.com>]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Thursday, June 9, 2022 3:55 PM
> To: Spike Du <spiked@nvidia.com>; Matan Azrad <matan@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> Xiaoyun Li <xiaoyun.li@intel.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
> Cc: stephen@networkplumber.org; mb@smartsharesystems.com;
> dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>
> Subject: Re: [PATCH v5 7/7] app/testpmd: add Host Shaper command
> 
> External email: Use caution opening links or attachments
> 
> 
> Since ethdev patch is factored out from the patch series the rest could go to
> mlx5 maintainers.
> 
> On 6/7/22 15:59, Spike Du wrote:
> > Add command line options to support host shaper configure.
> > - Command syntax:
> >    mlx5 set port <port_id> host_shaper avail_thresh_triggered <0|1>
> > rate <rate_num>
> >
> > - Example commands:
> > To enable avail_thresh_triggered on port 1 and disable current host
> > shaper:
> > testpmd> mlx5 set port 1 host_shaper avail_thresh_triggered 1 rate 0
> >
> > To disable avail_thresh_triggered and current host shaper on port 1:
> > testpmd> mlx5 set port 1 host_shaper avail_thresh_triggered 0 rate 0
> >
> > The rate unit is 100Mbps.
> > To disable avail_thresh_triggered and configure a shaper of 5Gbps on
> > port 1:
> > testpmd> mlx5 set port 1 host_shaper avail_thresh_triggered 0 rate 50
> >
> > Add sample code to handle rxq available descriptor threshold event, it
> > delays a while so that rxq empties, then disables host shaper and
> > rearms available descriptor threshold event.
> >
> > Signed-off-by: Spike Du <spiked@nvidia.com>
> 
> [snip]
> 
> > +
> > +static uint8_t host_shaper_avail_thresh_triggered[RTE_MAX_ETHPORTS];
> > +#define SHAPER_DISABLE_DELAY_US 100000 /* 100ms */
> > +
> > +/**
> > + * Disable the host shaper and re-arm available descriptor threshold event.
> > + *
> > + * @param[in] args
> > + *   uint32_t integer combining port_id and rxq_id.
> > + */
> > +static void
> > +mlx5_test_host_shaper_disable(void *args) {
> > +     uint32_t port_rxq_id = (uint32_t)(uintptr_t)args;
> > +     uint16_t port_id = port_rxq_id & 0xffff;
> > +     uint16_t qid = (port_rxq_id >> 16) & 0xffff;
> > +     struct rte_eth_rxq_info qinfo;
> > +
> > +     printf("%s disable shaper\n", __func__);
> > +     if (rte_eth_rx_queue_info_get(port_id, qid, &qinfo)) {
> > +             printf("rx_queue_info_get returns error\n");
> > +             return;
> > +     }
> > +     /* Rearm the available descriptor threshold event. */
> > +     if (rte_eth_rx_avail_thresh_set(port_id, qid, qinfo.avail_thresh)) {
> > +             printf("config avail_thresh returns error\n");
> > +             return;
> > +     }
> > +     /* Only disable the shaper when avail_thresh_triggered is set. */
> > +     if (host_shaper_avail_thresh_triggered[port_id] &&
> > +         rte_pmd_mlx5_host_shaper_config(port_id, 0, 0))
> > +             printf("%s disable shaper returns error\n", __func__); }
> > +
> > +void
> > +mlx5_test_avail_thresh_event_handler(uint16_t port_id, uint16_t
> > +rxq_id) {
> > +     uint32_t port_rxq_id = port_id | (rxq_id << 16);
> 
> Nobody guarantees here that port_id refers to an mlx5 port.
> It could be any port with avail_thres support.
I think at the beginning of this function, we can add below check:

  if (rte_eth_dev_info_get(port_id, &dev_info) != 0 ||
      (strncmp(dev_info.driver_name, "mlx5", 4) != 0))
	return;

is it ok?
> 
> > +
> > +     rte_eal_alarm_set(SHAPER_DISABLE_DELAY_US,
> > +                       mlx5_test_host_shaper_disable,
> > +                       (void *)(uintptr_t)port_rxq_id);
> > +     printf("%s port_id:%u rxq_id:%u\n", __func__, port_id, rxq_id);
> > +}
> 
> [snip]
> 
> > +cmdline_parse_token_string_t cmd_port_host_shaper_mlx5 =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_port_host_shaper_result,
> > +                             mlx5, "mlx5");
> 
> I think it lucks 'static' keyword (many cases below).

Sure, will add 'static'.
> 
> [snip]