On 10/29/26 9:42 AM, Eelco Chaudron wrote:
>
>
> On 28 Oct 2026, at 18:26, David Marchand via dev wrote:
>
>> On Thu, 8 Oct 2026 at 15:36, Eelco Chaudron <[email protected]> wrote:
>>> [...]
>>>
>>>> + if (policer) {
>>>> + ovsrcu_postpone__((void (*)(void *))
>>>> dpdk_qos_ingress_policer_destruct,
>>>> + policer);
>>>
>>> Could this use ovsrcu_postpone() directly? The types
>>> match: dpdk_qos_ingress_policer_destruct() takes a
>>> 'struct dpdk_qos_ingress_policer *' and 'policer' is
>>> the same type.
>>
>> Indeed.
>> This is probably a leftover of some experiment I did.
>> I can probably call free() like in original code, I'll double check.
>>
>>
>>>> diff --git a/lib/netdev-dpdk-common.h b/lib/netdev-dpdk-common.h
>>>
>>> [...]
>>>
>>>> +void dpdk_qos_ingress_policer_destruct(struct dpdk_qos_ingress_policer *);
>>>> +int dpdk_qos_ingress_policer_run(struct dpdk_qos_ingress_policer *policer,
>>>
>>> Per the coding style, parameter names can be omitted
>>> from prototypes when the type already makes the purpose
>>> clear. For instance 'policer' here, 'common' below, and
>>> similar struct pointer parameters throughout this move.
>>> Also true for earlier patches.
>>
>> Well, I just moved code.
>>
>> On the other hand, every time I submit a patch with "omitted" names, I
>> get Ilya asking me to add explicit names :-).
>> So please, give me an explicit list of things to adjust, else I prefer
>> to leave as is.
If it's an 'int' or a 'bool', then it needs a name, as it is not really
possible to say what is the meaning of it. If it's something like the
'struct dpdk_qos_ingress_policer *', and it is the only argument of that
type, then it's self-explanatory in most cases and the name should be
omitted.
>
> See my diff below. @Iyla, what are your thoughts on this? I feel like this
> is a new file, so we might as well fix it now.
I think, yes. Since we're touching this code anyway, we may as well make
it a little bit better. Large refactorings on move are not good, but small
style adjustments should be fine.
>
> //Eelco
>
>
> diff --git a/lib/netdev-dpdk-common.h b/lib/netdev-dpdk-common.h
> index a9bba274a..91067ebf7 100644
> --- a/lib/netdev-dpdk-common.h
> +++ b/lib/netdev-dpdk-common.h
> @@ -45,12 +45,10 @@ extern struct ovs_mutex dpdk_vhost_mutex;
> extern struct ovs_mutex dpdk_mp_mutex
> OVS_ACQ_AFTER(dpdk_eth_mutex, dpdk_vhost_mutex);
>
> -/*
> - * need to reserve tons of extra space in the mbufs so we can align the
> +/* Need to reserve tons of extra space in the mbufs so we can align the
> * DMA addresses to 4KB.
> * The minimum mbuf size is limited to avoid scatter behaviour and drop in
> - * performance for standard Ethernet MTU.
> - */
> + * performance for standard Ethernet MTU. */
> #define ETHER_HDR_MAX_LEN (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN \
> + (2 * VLAN_HEADER_LEN))
> #define MTU_TO_FRAME_LEN(mtu) ((mtu) + RTE_ETHER_HDR_LEN + \
> @@ -165,18 +163,16 @@ enum dpdk_hw_ol_features {
> NETDEV_TX_GRE_TNL_TSO_OFFLOAD = 1 << 12,
> };
>
> -void netdev_dpdk_common_update_ol_flags(struct netdev_dpdk_common *common);
> +void netdev_dpdk_common_update_ol_flags(struct netdev_dpdk_common *);
>
> -int netdev_dpdk_common_get_numa_id(const struct netdev *netdev);
> -void netdev_dpdk_common_get_etheraddr(const struct netdev_dpdk_common
> *common,
> - struct eth_addr *mac);
> -void netdev_dpdk_common_get_mtu(const struct netdev_dpdk_common *common,
> - int *mtup);
> -int netdev_dpdk_common_get_ifindex(const struct netdev_dpdk_common *common);
> -int netdev_dpdk_common_set_mtu(struct netdev_dpdk_common *common, int mtu);
> -void
> -netdev_dpdk_common_get_sw_custom_stats(struct netdev_dpdk_common *common,
> - struct netdev_custom_stats *stats);
> +int netdev_dpdk_common_get_numa_id(const struct netdev *);
> +void netdev_dpdk_common_get_etheraddr(const struct netdev_dpdk_common *,
> + struct eth_addr *);
> +void netdev_dpdk_common_get_mtu(const struct netdev_dpdk_common *, int
> *mtup);
> +int netdev_dpdk_common_get_ifindex(const struct netdev_dpdk_common *);
> +int netdev_dpdk_common_set_mtu(struct netdev_dpdk_common *, int mtu);
> +void netdev_dpdk_common_get_sw_custom_stats(struct netdev_dpdk_common *,
> + struct netdev_custom_stats *);
>
> /* Allocates an area of 'sz' bytes from DPDK. The memory is zero'ed.
> *
> @@ -199,68 +195,67 @@ struct dpdk_mp_config {
>
> void dpdk_mp_init(const struct smap *ovs_other_config);
> bool dpdk_mp_per_port_memory(void);
> -struct dpdk_mp * dpdk_mp_get(struct dpdk_mp_config *cfg);
> -void dpdk_mp_put(struct dpdk_mp *dmp);
> -void dpdk_mp_dump(FILE *stream, struct dpdk_mp *dmp);
> +struct dpdk_mp * dpdk_mp_get(struct dpdk_mp_config *);
May want to clean up that extra space after the star as well.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
