Re: [vpp-dev] 11th hour changes and code reviews
> On May 4, 2020, at 3:46 PM, Andrew Yourtchenko wrote: > More inline: >> On 4 May 2020, at 19:20, Paul Vinciguerra wrote: >> >> I agree that the change I submitted is a monster. Honestly, there are 2-5 >> git-friendly changes that could be refactored out of it. Its written and >> used by me, but it's not worth investing the time to make mergeable if there >> is no desire for it. > > That’s why it’s handy to discuss the big changes that touch the APIs upfront, > with the end users. Just last week I saw a mail on how the past year there > were so many API changes, which make life harder. VPP’s reason for existence > (at least in my view) is its users. > > This change reshuffles the messages and changes semantics for a bunch of > calls for the sole purpose of architectural purity. Purity is beautiful but > the users deserve a heads up at least. > >> I only submitted it at all because I had mentioned last week to someone >> that I had essentially stopped contributing and I was asked if I could be >> enticed to start re-submitting code. There is no intent on my part to >> subvert the release. >> >> Everyone complains about the tests. I was "asked" last year to clean up the >> tests. It's not that code hasn't been written to clean it up. The reality >> is that code becomes a monster when simple refactorings aren't merged. > > The tests need proper debugging first, *before* doing the refactorings. The > tests still can not run with high concurrent number of jobs. (N~48). > The tests still fail intermittently. > > This is what folks mean when when complaining. Rearranging the calling > convention doesn’t help this in the slightest, just adds unnecessary churn. > >> >> If you all tell me you want the changes, I'm glad to clean it up. > > That change contains multiple pieces which you yourself described. > > - dump by sw_if_index ? Sure. But again, not across the entire code base but > component by component (if there are many). Reviewed by the maintainer of the > component. The fact that this might be a bit painful to do in parts is a nice feedback to the pain that the change causes downstream. :) I like consistent APIs as much as the next person, maybe more so, but non-backward compatible changes in order to tidy up the API cause pain and maybe aren't so great. That said, adding filter functionality is good, so if there wasn't the ability to filter on sw_if_index before, I'd vote for it. Thanks, Chris. > > - changing the message structure ? Again, if there are multiple components, > it should not be hard making small changes that would be trivial to review > (though before that, like i said, it would be nice to prove the users and > subsystem maintainers what they think of the changes). Also how do we ensure > the “old” inconsistency doesn’t resurrect in the new messages ? If there is > no automated check against that, this is just adding churn. > > - internal refactorings that have zero user impact. Why not. But again, if > you are going across components, each commit should be within a component, > and reviewed/committed by the maintainer of the component. > > —a > >> >> If, in the meantime, someone wants to +2 Neale's change [2], that would be >> great! >> >> [0] https://gerrit.fd.io/r/c/vpp/+/26833 >> [1] https://lists.fd.io/g/csit-dev/message/4009 >> [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 > -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16236): https://lists.fd.io/g/vpp-dev/message/16236 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] Resources for learning about VOM
Does anything exist beyond the description on the wiki? -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16235): https://lists.fd.io/g/vpp-dev/message/16235 Mute This Topic: https://lists.fd.io/mt/73983633/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] 11th hour changes and code reviews
Thanks for writing! Since i was the “-2“, first exec summary and then points inline: - CRC changing changes in 10 unrelated .api files at this point in time are borderline hostile activity to downstream users. Every CRC change means changes are required downstream. Sure it’s master but some folks are also preparing the releases dependent on VPP, and given the times I prefer to limit the changes that don’t correspond to obvious end-user-benefiting features. - as someone who has to do cherrypicks regularly, I can say that commits like this, which span multitude of files and yet include multiple orthogonal fixes are making this job much harder. This means that for any downstream commit that is unrelated to this but happens to touch one of the changes lines, I will need to open up the massive commit, and figure out which part does which, etc. More inline: >> On 4 May 2020, at 19:20, Paul Vinciguerra wrote: > I was -2'd this morning for submitting a change with a "blast radius" this > close to api freeze [0]. It was suggested that it should be discussed on the > dev list and that multiple parties outside of vpp should have to +1 it. I am > glad to start the conversation. > > My change is dependent on another developer's change has been sitting > waiting. for a response from csit-dev almost a week. I asked last week if it > would be ok to refactor out the non-csit related code, but got no response. > I refactored the dependent code out of the other change, but it is still > outstanding. That change probably is another api breaking change, I presume. The more api breaking changes are in the CSIT pipeline, the slower they will respond. But it’s speculation. We just did the 19.08.2 so they might be busy also testing that. > > I agree that the change I submitted is a monster. Honestly, there are 2-5 > git-friendly changes that could be refactored out of it. Its written and > used by me, but it's not worth investing the time to make mergeable if there > is no desire for it. That’s why it’s handy to discuss the big changes that touch the APIs upfront, with the end users. Just last week I saw a mail on how the past year there were so many API changes, which make life harder. VPP’s reason for existence (at least in my view) is its users. This change reshuffles the messages and changes semantics for a bunch of calls for the sole purpose of architectural purity. Purity is beautiful but the users deserve a heads up at least. > I only submitted it at all because I had mentioned last week to someone > that I had essentially stopped contributing and I was asked if I could be > enticed to start re-submitting code. There is no intent on my part to > subvert the release. > > Everyone complains about the tests. I was "asked" last year to clean up the > tests. It's not that code hasn't been written to clean it up. The reality > is that code becomes a monster when simple refactorings aren't merged. The tests need proper debugging first, *before* doing the refactorings. The tests still can not run with high concurrent number of jobs. (N~48). The tests still fail intermittently. This is what folks mean when when complaining. Rearranging the calling convention doesn’t help this in the slightest, just adds unnecessary churn. > > If you all tell me you want the changes, I'm glad to clean it up. That change contains multiple pieces which you yourself described. - dump by sw_if_index ? Sure. But again, not across the entire code base but component by component (if there are many). Reviewed by the maintainer of the component. - changing the message structure ? Again, if there are multiple components, it should not be hard making small changes that would be trivial to review (though before that, like i said, it would be nice to prove the users and subsystem maintainers what they think of the changes). Also how do we ensure the “old” inconsistency doesn’t resurrect in the new messages ? If there is no automated check against that, this is just adding churn. - internal refactorings that have zero user impact. Why not. But again, if you are going across components, each commit should be within a component, and reviewed/committed by the maintainer of the component. —a > > If, in the meantime, someone wants to +2 Neale's change [2], that would be > great! > > [0] https://gerrit.fd.io/r/c/vpp/+/26833 > [1] https://lists.fd.io/g/csit-dev/message/4009 > [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16234): https://lists.fd.io/g/vpp-dev/message/16234 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] accessing pool entries in gdb
In [live, not core-file] gdb, try this: (gdb) p pifi(pool, ) Which will tell you if is free (invalid) or not (valid). Also: (gdb) p pool_elts(pool) To see how many elements are in the pool. Finally: (gdb) p vl(pool) To what vec_len(pool) is. HTH... Dave From: vpp-dev@lists.fd.io On Behalf Of Satya Murthy Sent: Monday, May 4, 2020 3:13 PM To: vpp-dev@lists.fd.io Subject: [vpp-dev] accessing pool entries in gdb Having some issue while accessing entries of a pool in GDB. I have a pool of some structures. custom_struct *pool; This custom_struct has alignment with 64 byte cache_line. now, I have added 3 entries in this pool. The code seems to be working fine in adding/deleting/traversing this pool using pool_elt_at_index. However, If i use gdb to view the elements using pool[0], pool[1] and pool[2], they are giving some invalid entries. The first entry pool[0] seem to be fine, but the next entry onwards are showing invalid entries in GDB. Is there anything wrong i am doing in gdb while traversing the pools ? -- Thanks & Regards, Murthy -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16233): https://lists.fd.io/g/vpp-dev/message/16233 Mute This Topic: https://lists.fd.io/mt/73982726/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] accessing pool entries in gdb
Having some issue while accessing entries of a pool in GDB. I have a pool of some structures. custom_struct *pool; This custom_struct has alignment with 64 byte cache_line. now, I have added 3 entries in this pool. The code seems to be working fine in adding/deleting/traversing this pool using pool_elt_at_index. However, If i use gdb to view the elements using pool[0], pool[1] and pool[2], they are giving some invalid entries. The first entry pool[0] seem to be fine, but the next entry onwards are showing invalid entries in GDB. Is there anything wrong i am doing in gdb while traversing the pools ? -- Thanks & Regards, Murthy -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16232): https://lists.fd.io/g/vpp-dev/message/16232 Mute This Topic: https://lists.fd.io/mt/73982726/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] 11th hour changes and code reviews
I was -2'd this morning for submitting a change with a "blast radius" this close to api freeze [0]. It was suggested that it should be discussed on the dev list and that multiple parties outside of vpp should have to +1 it. I am glad to start the conversation. My change is dependent on another developer's change has been sitting waiting. for a response from csit-dev almost a week. I asked last week if it would be ok to refactor out the non-csit related code, but got no response. I refactored the dependent code out of the other change, but it is still outstanding. I agree that the change I submitted is a monster. Honestly, there are 2-5 git-friendly changes that could be refactored out of it. Its written and used by me, but it's not worth investing the time to make mergeable if there is no desire for it. I only submitted it at all because I had mentioned last week to someone that I had essentially stopped contributing and I was asked if I could be enticed to start re-submitting code. There is no intent on my part to subvert the release. Everyone complains about the tests. I was "asked" last year to clean up the tests. It's not that code hasn't been written to clean it up. The reality is that code becomes a monster when simple refactorings aren't merged. If you all tell me you want the changes, I'm glad to clean it up. If, in the meantime, someone wants to +2 Neale's change [2], that would be great! [0] https://gerrit.fd.io/r/c/vpp/+/26833 ( https://gerrit.fd.io/r/c/vpp/+/26833 ) [1] https://lists.fd.io/g/csit-dev/message/4009 ( https://lists.fd.io/g/csit-dev/message/4009 ) [2] https://gerrit.fd.io/r/c/vpp/+/26820/4 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16231): https://lists.fd.io/g/vpp-dev/message/16231 Mute This Topic: https://lists.fd.io/mt/73980355/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] route lookup api
After multiple amended commits to fix python style errors, a new version with v4/v6 API tests has been uploaded. https://gerrit.fd.io/r/c/vpp/+/26829 Thanks, Chris. > On May 2, 2020, at 5:25 PM, Christian Hopps wrote: > > I was able to do a minimal test on it so I submitted, but please review as > well. :) > > https://gerrit.fd.io/r/c/vpp/+/26829 > > Thanks, > Chris. > >> On Apr 28, 2020, at 12:08 PM, Jon Loeliger via lists.fd.io >> wrote: >> >> Chris, >> >> If you can shoot me the patch, as is, I will rebase and submit it? >> >> jdl >> >> >> On Mon, Apr 27, 2020 at 7:27 PM Dave Wallace wrote: >> Gentle reminder that the VPP 20.05 API freeze is next Wed, May 6, 2020, so >> please don't delay to long if this is required for 20.05. >> >> Thanks, >> -daw- (wearing the friendly VPP release manager hat) >> >> On 4/27/2020 5:02 PM, Christian Hopps wrote: On Apr 27, 2020, at 11:44 AM, Jon Loeliger via lists.fd.io wrote: On Wed, Feb 19, 2020 at 4:47 AM Christian Hopps wrote: > On Feb 19, 2020, at 2:02 AM, Neale Ranns via Lists.Fd.Io > > wrote: > > > Hi Chris, > > Adding an API to dump a single route would be a welcome addition to the > API. > Ok, I'll do that then. Hey Chris, Did you ever get this API enhancement written? If so, is there a Gerrit for it? >>> I have this written/tested, but it lives in my 19.08 (product) branch right >>> now. I might have near the end of the week to cherry pick it to master and >>> test it there. >>> >>> Thanks, >>> Chris. >>> >>> Thanks, jdl >>> >>> >> >> >> > > -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16230): https://lists.fd.io/g/vpp-dev/message/16230 Mute This Topic: https://lists.fd.io/mt/71382541/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
(disregard the original mail) Re: [vpp-dev] VPP 20.01.1 is on Wednesday 06 May 2020
Appears I misunderstood the capacity planning WRT the CSIT testing - so please disregard my 20.01.1 mail this mail replies to. I will send an announcement separately when we finalize the date for the 20.01.1. --a (your friendly, even if slightly confused, 20.01 release manager) On 5/4/20, Andrew Yourtchenko via lists.fd.io wrote: > Hello all, > > From "stable/2001 branch" news department: we are planning to tag the > 20.01.1 release this Wednesday. > > Please let me know ASAP if you have some work in progress that must be > included in it. > > cheers, > andrew > (your friendly 20.01 release manager) > -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16229): https://lists.fd.io/g/vpp-dev/message/16229 Mute This Topic: https://lists.fd.io/mt/73977153/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] VPP 20.01.1 is on Wednesday 06 May 2020
Hello all, >From "stable/2001 branch" news department: we are planning to tag the 20.01.1 release this Wednesday. Please let me know ASAP if you have some work in progress that must be included in it. cheers, andrew (your friendly 20.01 release manager) -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16228): https://lists.fd.io/g/vpp-dev/message/16228 Mute This Topic: https://lists.fd.io/mt/73976653/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[vpp-dev] VPP 20.05: F0 (API freeze) milestone this wednesday 06 May 2020
Hi all, This Wednesday, 6th of May, is the scheduled date [0] for the F0 - API Freeze - milestone. At that point, until the new stable/2005 branch is pulled, there will be no API changes allowed on master branch, and we will limit the commits to only low-risk changes. Please drop me an email today or tomorrow if you have pending API changes that you need to be included into the VPP 20.05. cheers, andrew -- your friendly VPP 20.05 release manager [0] https://wiki.fd.io/view/Projects/vpp/Release_Plans/Release_Plan_20.05 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16227): https://lists.fd.io/g/vpp-dev/message/16227 Mute This Topic: https://lists.fd.io/mt/73976547/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] api_format.c
Hi Paul, et al,
> I've run into an api where the api was changed and the SCRIPT: output wasn't
> updated to match.
> I'd like to clean it up. What is the best way to verify that the api_format
> code is correct?
>
> Is a self.vapi.cli("binary-api foo") in a python unit test sufficient?
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
This is a somewhat thorny area.
VPP itself supports dumping of API traces in two formats (sort of). User
friendly and "SCRIPT" that's tuned to output API commands that can be used by
VAT.
Some VPP features support only the user friendly dump, some only the "SCRIPT"
format and some none.
I don't see any reason why we cannot auto-generate the dump/custom-dump code.
Either as external post-processing of API trace files, or for the "api trace
dump|custom-dump" commands in VPP.
Best regards,
Ole-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#16226): https://lists.fd.io/g/vpp-dev/message/16226
Mute This Topic: https://lists.fd.io/mt/73395667/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [vpp-dev] worker barrier state
Hi Chris,
With SAs there are two scenarios to consider for inflight packets
1) the SA is unlinked
2) the SA is deleted.
We've talked at length about how to deal with 2).
By 'unlinked' I mean that whatever config dictated that an SA be used has now
gone (like tunnel protection or SPD policy). An inflight packet that is
processed by an IPSec node would (by either scheme we discussed for 1))
retrieve the SA do encrypt/decrypt and then attempt to send the packet on its
merry way; this is the point at which it could fail. I say 'could' because it
depends on how the unlink affected the vlib graph. In today's tunnel protection
esp_encrpyt does vnet_feature_next(), this is not going to end well once
esp_encrypt is no longer a feature on the arc. In tomorrow's tunnel protection
we'll change that:
https://gerrit.fd.io/r/c/vpp/+/26265
and it should be safe. But, what if the next API removes the tunnel whilst
there are still inflight packets? Is it still safe? Is it still correct to send
encrypted tunnelled packets?
I think I'm coming round to the opinion that the safest way to approach this is
to ensure that if the SA can be found, whatever state it is in (created,
unlinked or deleted) then it needs to have a flag that states whether it should
be used or the packet dropped. We'd update this state when the SA is
[un]linked, with the barrier held.
On a somewhat related topic, you probably saw:
https://gerrit.fd.io/r/c/vpp/+/26811
as an example of getting MP safe APIs wrong.
/neale
On 24/04/2020 16:34, "Christian Hopps" wrote:
Hi Neale,
Comments also inline...
Neale Ranns (nranns) writes:
> Hi Chris,
>
> Comments inline...
>
> On 15/04/2020 15:14, "Christian Hopps" wrote:
>
> Hi Neale,
>
> I agree that something like 4, is probably the correct approach. I
had a side-meeting with some of the ARM folks (Govind and Honnappa), and we
thought using a generation number for the state rather than just waiting
"long-enough" to recycle it could work. The generation number would be the
atomic value associated with the state. So consider this API:
>
> - MP-safe pools store generation numbers alongside each object.
> - When you allocate a new object from the pool you get an index and
generation number.
> - When storing the object index you also save the generation number.
> - When getting a pointer to the object you pass the API the index
and generation number and it will return NULL if the generation number did not
match the one stored with the object in the pool.
> - When you delete a pool object its generation number is incremented
(with barrier).
>
> The size of the generation number needs to be large enough to
guarantee there is no wrap with objects still in the system that have stored
the generation number. Technically this is a "long-enough" aspect of the
scheme. :) One could imagine using less than 64 bits for the combination of
index and generation, if that was important.
>
> It's a good scheme, I like it.
> I assume the pool indices would be 64 bit and the separation between
vector index and generation would be hidden from the user. Maybe a 32 bit value
would suffice in most cases, but why skimp...
I was thinking to keep the index and generation number separate at the most
basic API, to allow for selecting the size of the each independently and for
efficient storage. I'm thinking for some applications one might want to do
something like
cacheline_packed_struct {
...
u32 foo_index;
u32 bar_index;
u16 foo_gen;
u16 bar_gen;
...
};
a set of general purpose macros could be created for combining the 2 values
into a single integer value though.
> The advantage over just waiting N seconds to recycle the index is
that the system scales better, i.e., if you just wait N seconds to reuse, and
are creating and deleting objects at a significant rate, your pool can blow up
in the N seconds of time. With the generation number this is not a problem as
you can re-use the object immediately. Another advantage is that you don't have
to have the timer logic (looping per pool or processing all pools) to free up
old indices.
>
> Yes, for my time based scheme, the size of the pool will become dependent
on some integration over a rate of change, which is not deterministic, which is
not great, but I don't suppose all APIs are subject to large churn.
> With the generation scheme the pool always requires more memory, since
you're storing a generation value for each index, but being a deterministic
size (even though probably bigger), I'd probably take that.
> I wouldn't use timer logic in my scheme. I'd make the pool's free-list a
fifo (as opposed to the stack it is today) and each entry in the list has the
index and the time it was added. If t_now - t_head < t_wrap I
