> From: fengchengwen [mailto:fengcheng...@huawei.com]
> Sent: Tuesday, 5 August 2025 04.10
> 
> On 8/4/2025 3:42 AM, Morten Brørup wrote:
> > When an ethdev port is configured for fast mbuf release, the driver can
> > use a TX burst function relying on the fast mbuf release preconditions.
> > Thus, also configuring this port or a queue on the port for transmitting
> > segmented packets is prohibited.
> > Checks for these conflicting configurations have been added to the ethdev
> > library, so the drivers don't have to implement them.
> 
> I think the new comment part of ombuf-fast-free and multi-seg in '[PATCH v3
> 2/3] ethdev: Improve descriptions of RX and TX offloads'
> could drop or within this commit.

Although the comments are new, I think the behavior described by the new 
comments is not really new, it was just undocumented.
So, patch 2/3 properly documents it, and patch 3/3 enforces it in the ethdev 
layer, so drivers don't have to.

> 
> >
> > Signed-off-by: Morten Brørup <m...@smartsharesystems.com>
> > Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> > Acked-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> > Acked-by: Konstantin Ananyev <konstantin.anan...@huawei.com>
> > ---
> > v3:
> > * Shorten the added log messages. (Stephen Hemminger, Ivan Malov)
> > ---
> >  lib/ethdev/rte_ethdev.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index dd7c00bc94..00afda873f 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -1531,6 +1531,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t
> nb_rx_q, uint16_t nb_tx_q,
> >             goto rollback;
> >     }
> >
> > +   /* MBUF_FAST_FREE preconditions conflict with MULTI_SEGS support. */
> > +   if ((dev_conf->txmode.offloads & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) &&
> > +       (dev_conf->txmode.offloads & RTE_ETH_TX_OFFLOAD_MULTI_SEGS)) {
> > +           RTE_ETHDEV_LOG_LINE(ERR,
> > +                   "id=%d offload clash, %s vs %s",
> 
> Suggest keep the same trace fmt. e.g.: Ethdev port_id=%u
> The same below

Initial versions of the series did that, but other reviewers requested a 
shorter log format.

> 
> > +                   port_id,
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MULTI_SEGS),
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE));
> > +           ret = -EINVAL;
> > +           goto rollback;
> > +   }
> > +
> >     dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf =
> >             rte_eth_rss_hf_refine(dev_conf->rx_adv_conf.rss_conf.rss_hf);
> >
> > @@ -2709,6 +2721,31 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t
> tx_queue_id,
> >             return -EINVAL;
> >     }
> >
> > +   /*
> > +    * If the driver uses a Tx function with MBUF_FAST_FREE preconditions,
> > +    * per-queue MULTI_SEGS support is not possible.
> > +    */
> > +   if ((dev->data->dev_conf.txmode.offloads &
> RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) &&
> > +                   (local_conf.offloads & RTE_ETH_TX_OFFLOAD_MULTI_SEGS)) {
> > +           RTE_ETHDEV_LOG_LINE(ERR,
> > +                   "id=%d txq=%d offload clash, per-queue %s vs per-port 
> > %s",
> > +                   port_id, tx_queue_id,
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MULTI_SEGS),
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE));
> > +           return -EINVAL;
> > +   }
> > +   /*
> > +    * If the driver uses a Tx function with MULTI_SEGS support,
> > +    * runtime support for per-queue MBUF_FAST_FREE optimization depends on
> the driver.
> > +    */
> > +   if ((dev->data->dev_conf.txmode.offloads &
> RTE_ETH_TX_OFFLOAD_MULTI_SEGS) &&
> > +                   (local_conf.offloads & 
> > RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE))
> > +           RTE_ETHDEV_LOG_LINE(DEBUG,
> > +                   "id=%d txq=%d potential offload clash, per-queue %s vs
> per-port %s: PMD to decide",
> > +                   port_id, tx_queue_id,
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE),
> > +
>       rte_eth_dev_tx_offload_name(RTE_ETH_TX_OFFLOAD_MULTI_SEGS));
> 
> 1\ The global port configuration will take effect on all queues.
>    If it don't support both MBUF_FAST_FREE and MULTI_SEG, why don't ban it
> here?

There is a slight difference...
If a port is configured for MBUF_FAST_FREE, it may install a TX function 
(port-wide, i.e. on all queues) relying on the mbuf-fast-free requirements, 
which prohibit segmented packets. So combining per-port MBUF_FAST_FREE with 
per-port or per-queue MULTI_SEGS is impossible.
However, if a port is configured for MULTI_SEGS, it may install a TX function 
(port-wide) with support for per-queue MBUF_FAST_FREE, by checking the queue 
structure at runtime.

> 2\ Debug level is easy to ignore, how about warning?

If a driver's installed MULTI_SEGS TX function supports runtime checking the 
queue for MBUF_FAST_FREE, then there should be no warning/notice level log 
message, as it is considered "normal" behavior.
However, if a driver doesn't support it, the driver must return -EINVAL for 
such a queue configuration request. And if the driver doesn't log any message 
with that, then this debug level log message could help the user debug the 
issue.

I was in doubt if I should add this debug check or not, but I decided that it 
might be useful for debugging driver issues related to such a configuration.

> 
> > +
> >     rte_ethdev_trace_txq_setup(port_id, tx_queue_id, nb_tx_desc, tx_conf);
> >     return eth_err(port_id, dev->dev_ops->tx_queue_setup(dev,
> >                    tx_queue_id, nb_tx_desc, socket_id, &local_conf));

Reply via email to