From: "Kuusisaari, Juhamatti (Coriant - FI/Espoo)" <juhamatti.kuusisaari@coriant.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v6] net/pcap: physical interface MAC address support
Date: Mon, 1 Oct 2018 07:30:15 +0000 [thread overview]
Message-ID: <HE1PR0402MB292125BCE5937D35BBF80F7D9DEF0@HE1PR0402MB2921.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <315802bc-db03-369e-2bb6-6a1cd34b9ffb@intel.com>
Hello Ferruh,
> On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote:
> > At the moment, PCAP interfaces use dummy MAC by default. This change
> > adds support for selecting PCAP physical interface MAC with phy_mac=1
> > devarg. This allows to setup packet flows using the physical interface
> > MAC.
> >
> > Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
> >
> > ---
> > v6:
> > * Review changes:
> > * Clarified devarg applicability (applies to iface-only)
> > * Introduced detailed documentation for the devarg
> > * More checkpatches-fixes
> > v5-v3:
> > * FreeBSD related compilation and checkpatches-fixes
> > v2:
> > * FreeBSD support introduced
> > * Release notes added
> > v1:
> > * phy_mac=1 devarg support
>
> <...>
>
> > +#elif defined(__FreeBSD__)
>
> Just to double check did you check/verify below code on FreeBSD?
I did run a manual test of the added code and seems to work. I think it is still better to run DPDK on it to make sure everything works, so I'll do that before sending next revision.
>
> <...>
>
> > @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,
> > else
> > (*internals)->if_index = if_nametoindex(pair->value);
> >
> > + if (phy_mac && pair) /* phy_mac arg is applied only to iface */
>
> Having this comment is good, but "iface" is so generic, it may be confusing for
> beyond this context, what about "only if iface devarg provided" kind of detail?
Yes, let's elaborate that.
> <...>
>
> > @@ -989,6 +1073,19 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
> > return 0;
> > }
> >
> > +static int
> > +select_phy_mac(const char *key, const char *value, void *extra_args)
> > +{
> > + if (extra_args) {
> > + const int phy_mac = atoi(value);
> > + int *enable_phy_mac = extra_args;
> > +
> > + if (phy_mac)
> > + *enable_phy_mac = 1;
> > + }
> > + return 0;
> > +}
>
> This is causing build error because of "key" not used, needs __rte_unused
> marker.
Yes, need to fix this.
> <...>
>
> > @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> > * reading / writing
> > */
> > if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
> > + /*
> > + * We check first whether we want to use phy MAC of the
> PCAP
> > + * interface.
> > + */
> > + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
>
> Do you need count check at all?
I think it is needed, as the arg may not exist there. At least to me calling process directly does not feel right, even if it would work.
> > + ret = rte_kvargs_process(kvlist,
> ETH_PCAP_PHY_MAC_ARG,
> > + &select_phy_mac, &phy_mac);
> > + if (ret < 0)
> > + goto free_kvlist;
> > + }
>
> I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this
> block is
> for "iface", so it makes more sense to me first verify it, later verify phy_mac
Sure, I'll add it.
As there is already pcap-related MAC address patch merged, I'll need to do a rebase and recheck this patch functionality on top of that. It will take some time (which is a scarce resource at the moment), so I'll try to get this in again after 18.11.
Thanks,
--
Juhamatti
next prev parent reply other threads:[~2018-10-01 7:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-06 16:56 [dpdk-dev] [PATCH v5] " Juhamatti Kuusisaari
2018-09-10 12:03 ` Ferruh Yigit
2018-09-10 17:20 ` Kuusisaari, Juhamatti (Coriant - FI/Espoo)
2018-09-10 16:55 ` [dpdk-dev] [PATCH v6] " Juhamatti Kuusisaari
2018-09-11 15:35 ` Ferruh Yigit
2018-10-01 7:30 ` Kuusisaari, Juhamatti (Coriant - FI/Espoo) [this message]
2018-10-05 20:27 ` [dpdk-dev] [PATCH v7] " Ferruh Yigit
2018-10-06 0:49 ` [dpdk-dev] [PATCH v8] " Ferruh Yigit
2018-10-08 12:14 ` Ferruh Yigit
2018-10-09 4:30 ` Kuusisaari, Juhamatti (Infinera - FI/Espoo)
2018-10-09 12:04 ` Ferruh Yigit
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=HE1PR0402MB292125BCE5937D35BBF80F7D9DEF0@HE1PR0402MB2921.eurprd04.prod.outlook.com \
--to=juhamatti.kuusisaari@coriant.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.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).