10/02/2020 15:51, Jerin Jacob:
> On Tue, Feb 4, 2020 at 9:32 PM Gaetan Rivet <gr...@u256.net> wrote:
> >
> > On 04/02/2020 16:06, Thomas Monjalon wrote:
> > > 04/02/2020 13:43, Gaetan Rivet:
> > >> On 04/02/2020 12:07, Thomas Monjalon wrote:
> > >>> 04/02/2020 11:03, Gaetan Rivet:
> > >>>> On 03/02/2020 23:21, Thomas Monjalon wrote:
> > >>>>> 03/02/2020 06:16, Pavan Nikhilesh Bhagavatula:
> > >>>>>> @David Marchand @tho...@monjalon.net
> > >>>>>>
> > >>>>>> Ping?
> > >>>>>>
> > >>>>>> Are there any more changes required for this patch? It's been in
> > >>>>>> queue since last October.
> > >>>>>
> > >>>>> Sorry we have not decided whether it is a good idea or not.
> > >>>>>
> > >>>>> All changes related to probing are very sensitive,
> > >>>>> and we know a big refactoring would be better than stacking
> > >>>>> more and more options and corner cases.
> > >>>>>
> > >>>>> As we are busy with ABI stability stuff, we did not allocate
> > >>>>> enough time to properly think about this feature.
> > >>>>> Please accept our apologies, and let's consider it as
> > >>>>> a high priority for 20.05 cycle.
> > >>>>>
> > >>>>
> > >>>> Hello Thomas,
> > >>>>
> > >>>> This is unfortunate. I pushed Pavan to accept an alternative
> > >>>> implementation of this functionality that was less obtrusive, to make
> > >>>> the integration smoother. I took care to alleviate those risks from
> > >>>> the common path.
> > >>>>
> > >>>> The big refactoring is needed yes, but considering the current path
> > >>>> I'm not seeing it happen in 20.05. If that means taking this patch
> > >>>> as-is in 20.05 for Marvell users, I'm not sure much is gained from
> > >>>> waiting 3 months, except minimal risk avoidance.
> > >>>
> > >>>
> > >>> Yes, life is full of bad decisions and consequences.
> > >>
> > >>
> > >> Ah, yes, but I stand by my initial opinion, the first implementation [1]
> > >> was riskier and less useful.
> > >>
> > >>>
> > >>> I still think there is a risk in adding new user expectations,
> > >>> and maintaining some code to workaround unknown issues.
> > >>>
> > >>> The real question here is to know why this patch?
> > >>> Is it to workaround a broken driver?
> > >>> Or to workaround a broken design in EAL and bus drivers?
> > >>
> > >> Two birds - one stone here: OVS needed a way to disable automatic
> > >> probing cleanly (current workaround seen in multiple deployment is to
> > >> add a dummy whitelisted device, which will be ignored by the PCI bus -->
> > >> it sets the bus in whitelist mode but avoid probing anything), and as a
> > >> bonus this option allows using devices that depends on other devices
> > >> being probed already (LAG, representors, failsafe, etc).
> > >>
> > >> I'm not sure having a dependent-probe by default is good, and that would
> > >> be a big change.
> > >>
> > >> If we are doing the genesis of this patch, the initial motivation should
> > >> be asked for more details from Marvell people and David for the OVS side.
> > >>
> > >> [1]: First proposal:
> > >> http://mails.dpdk.org/archives/dev/2019-September/144166.html
> > >> My arguments:
> > >> http://mails.dpdk.org/archives/dev/2019-September/144564.html
> > >
> > >
> > > OK so there are two needs:
> > >
> > > 1/ Better control whitelist/blacklist mode.
> > > We already know that a rework is needed here.
> > > Unfortunately neither you or me had time to work on it,
> > > and others who were interested disappeared.
> > >
> > > 2/ Associate ports with equivalent properties in applications.
> > > This must be done in applications.
> > > Tweaking the probe order is a hack.
> > >
> > >
> >
> > An application that want to tightly control the port init order, currently
> > (by doing exactly like me here, hotpluging one by one the ports), would
> > still need the bigger hack that consist in inserting a whitelist PCI
> > devargs with a dummy address, depending on a undocumented PCI bus feature
> > consisting in ignoring matching errors but keeping probing policy from
> > failed devargs processing.
> >
> > Instead, with this patch this app can do
> >
> > rte_eal_manual_probe_set(1);
> > rte_eal_init();
> >
> > to have the same behavior and be able to hotplug ports as it sees fit.
> >
> > You are worried about creating user expectations about this behavior (being
> > forced to replicate in some way the functionality during the rewrite, as I
> > understand it?), but then you are currently forcing users to expect this
> > workaround to exist in the PCI bus, blocking devs from touching it as it
> > will thus break current app configurations. I've seen systemd unit file
> > using this -w dummy flag, as well as the programmatic equivalent. Which is
> > better, to have to rework it cutting short these configs, or to propose
> > beforehand a deprecation path?.
> >
> > This rework won't happen in 20.05, nor in the medium term unless you decide
> > to drive this change. This workaround serves three needs (PCI
> > normalization, port congruence and port dependency) in a low-risk
> > implementation.
>
> Thomas,
>
> What would be the resolution of this? What is your recommendation to
> fix the issue as you have the concern of this patch?
>
> Issue:
> 1) When l2fwd does the forwarding for simplicity and performance
> reason it just xor the port to find the destination port to forward.
> 2) If the adjacent ports are not symmetrical(example: one is 40G and
> other 10G) then forwarding will drop the packets.
>
> So, either
> a) We need to control the probing order
>
> b) Or Application need
> 1) To track the symmetrical ports and maintain the forwarding table OR
> 2) Have the command-line option to specify destination port like l3fwd.
>
> We can fix it in the application, but do we need to complicate l2fwd?
> I am fine with that, if that is consensus.
You are describing an application issue,
that's why I believe it should be fixed in the application.
Should we have a command line option to configure the forwarding rules
in the application (2)? I think yes.
Should we implement an application logic to automatically create
the best forwarding rules (1)? It would be nice, but anyway,
I think we need manual config (2) as a fallback.
> Thoughts? If you think, there is a rework needed in eal then could you
> enumerate the items for the rework.
Sorry I don't have time to describe dive into EAL probing and
enumerate the items to rework.
The most important issues I remind are:
- white/blacklist policy is a mess and should be done in a higher layer
- devargs syntax should allow generic matching (thanks to class
awareness and generic syntax)
Starting from these 2 items, we could imagine a generic path to
disable automatic probing, but I think the l2fwd logic should not
rely on probing order anyway.
