From: Shahaf Shuler <shahafs@mellanox.com>
To: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Yongseok Koh <yskoh@mellanox.com>,
"Adrien Mazarguil" <adrien.mazarguil@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop
Date: Mon, 23 Apr 2018 07:57:36 +0000 [thread overview]
Message-ID: <DB7PR05MB4426452F40DDF20B0097D4C2C3890@DB7PR05MB4426.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20180423073307.j7m7bdl4yguoikff@laranjeiro-vm.dev.6wind.com>
Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
[...]
> > > +/**
> > > + * DPDK callback to set multicast addresses list.
> > > + *
> > > + * @param dev
> > > + * Pointer to Ethernet device structure.
> > > + * @param mac_addr_set
> > > + * Multicast MAC address pointer array.
> > > + * @param nb_mac_addr
> > > + * Number of entries in the array.
> > > + *
> > > + * @return
> > > + * 0 on success, a negative errno value otherwise and rte_errno is set.
> > > + */
> > > +int
> > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > + struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > > + uint32_t i;
> > > + int ret;
> > > +
> >
> > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> before we start
> > operate.
>
> This verification is done in the sub function.
I see only assert. Did I missed anything?
>
> Considering an application calling such API wants to remove/replace the old
> list with new entries.
> That this new one can be just an addition or totally different list or even
> empty.
> This new list can be larger than the amount of MAC addresses the PMD can
> support.
>
> There are two possibilities:
>
> 1. The list is too large: the application will enable the all multicast mode to
> receive the extra mac addresses it needs.
How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES
> 2. The list fits (or empty): no issues.
>
> At the end the application can also call this API with an empty list to clear it
> before/after enabling the "all multicast" mode.
> The final result being the same, does it worse to add a duplicated
> verification?
At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault.
>
> Note: if an error happens the new list is not committed yet i.e. the traffic
> remains untouched.
>
> > > + for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > + mlx5_internal_mac_addr_remove(dev, i);
> > > + i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > + while (nb_mc_addr--) {
> >
> > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > + warn if it is not.
>
> Such verification should be done in the public API i.e. ethdev.
I don't understand.
In the first patch of the series you add extra verification to check the mac address validity. But for the MC you claim it should be done on ethdev layer.
It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch.
>
> > > + ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > i++);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > + if (!dev->data->promiscuous)
> > > + return mlx5_traffic_restart(dev);
> > > + return 0;
> > > +}
> > > --
> > > 2.17.0
>
> Regards,
>
> --
> Nélio Laranjeiro
> 6WIND
next prev parent reply other threads:[~2018-04-23 7:57 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-18 11:24 [dpdk-dev] [PATCH 0/2] implement set_mc_addr devop Nelio Laranjeiro
2018-04-18 11:24 ` [dpdk-dev] [PATCH 1/2] net/mlx5: split MAC address add/remove code Nelio Laranjeiro
2018-04-18 11:24 ` [dpdk-dev] [PATCH 2/2] net/mlx5: implement multicast add list devop Nelio Laranjeiro
2018-04-18 13:50 ` [dpdk-dev] [PATCH v2 0/3] net/mlx5: implement set_mc_addr devop Nelio Laranjeiro
2018-04-18 14:43 ` Adrien Mazarguil
2018-04-23 11:09 ` [dpdk-dev] [PATCH v3 " Nelio Laranjeiro
2018-04-23 12:46 ` Shahaf Shuler
2018-04-23 11:09 ` [dpdk-dev] [PATCH v3 1/3] net/mlx5: more checks on MAC addresses Nelio Laranjeiro
2018-04-23 11:09 ` [dpdk-dev] [PATCH v3 2/3] net/mlx5: split MAC address add/remove code Nelio Laranjeiro
2018-04-23 11:09 ` [dpdk-dev] [PATCH v3 3/3] net/mlx5: implement multicast add list devop Nelio Laranjeiro
2018-04-18 13:50 ` [dpdk-dev] [PATCH v2 1/3] net/mlx5: more checks on MAC addresses Nelio Laranjeiro
2018-04-18 13:50 ` [dpdk-dev] [PATCH v2 2/3] net/mlx5: split MAC address add/remove code Nelio Laranjeiro
2018-04-18 13:50 ` [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop Nelio Laranjeiro
2018-04-23 5:52 ` Shahaf Shuler
2018-04-23 7:33 ` Nélio Laranjeiro
2018-04-23 7:57 ` Shahaf Shuler [this message]
2018-04-23 9:34 ` Nélio Laranjeiro
2018-04-23 9:58 ` Shahaf Shuler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB7PR05MB4426452F40DDF20B0097D4C2C3890@DB7PR05MB4426.eurprd05.prod.outlook.com \
--to=shahafs@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.com \
--cc=yskoh@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).