On Mon, Feb 5, 2018 at 9:29 AM, John Lo (loj) <l...@cisco.com> wrote:
> Hi Jon,
>
Hi John,
> I don’t think the renumber scheme, as used by vhost and tap interfaces, is
> foolproof and can cause problems if used extensively for vxlan. On
> creating an interface with renumber, it does not check if the instance
> being requested is already used by another device. On creating an interface
> without renumber, it also does not check if the “natural instance’ may have
> already been used by another device’s renumber. Thus, on instance
> collision, last interface will take over the interface name and leave the
> previous interface not accessible by the duplicated interface name.
>
I was solving that the UI level, but it could be added into the
vxlan_add_del_interface() layer as well.
> I think we must check and reject instance misuse for this to be
> acceptable. I doubt the renumber is really used much for vhost and tap
> interface (others please correct me if not true).
>
We do use them on every call we make to these functions.
As well as on loopbacks. Knowing what object (by name) will b
created is essential for us.
> I see a “FIXME” warning in the existing vhost renumber function:
>
>
>
> static int
>
> vhost_user_name_renumber (vnet_hw_interface_t * hi, u32 new_dev_instance)
>
> {
>
> // FIXME: check if the new dev instance is already used
>
> vhost_user_main_t *vum = &vhost_user_main;
>
> vec_validate_init_empty (vum->show_dev_instance_by_real_dev_instance,
>
> hi->dev_instance, ~0);
>
Hmmm, yes, I see ...
> In your previous patch set #1, the usage of a bitmap to track instance
> usage seem a reasonable solution. It does imply we will never use “natural”
> instance and would always use the instance allocated from the bitmap and
> stored in the tunnel struct.
>
This procedure fails for some reason on the 5th test in VXLAN tests.
I am not certain, but I think it is because the renumbering effort was
trying to get the desired instance number out of the tunnel struct,
which lead to finding the corresponding vnet_hw_interface, and when
it needed to renumber, the hi->hw_if_index or hi->dev_instance wasn't
yet set, and it ended up creating IFs with wrong names (always 0?).
That said, we can combine the two approaches. That is, use the
show_instance_by_real_instance vector for knowing what name to use,
but also use a bitmap to record what was used and allocated.
One question on the bitmap approach. It had a hard-coded upper limit of 16K
bits in my previous patch. Are people OK with that? It is arbitrary, and
could
be larger. It simply depends on how much memory folks are willing to spend
recording in-use bits. That or a search of the show_by_real vector. Or we
could convert to a hash-map to check "in-use" status.
Preference?
As for the API/CLI, I though another parameter “instance” would suffice as
> ~0 can be used to state “not specified” rather than having an additional
> parameter “renumber”, although I can accept either way.
>
Two things here. I'm fine with either as well. I was following the
existing
pattern for the "renumber" approach. Using ~0 effectively means that the
message constructors must all explicitly initialize this field to ~0. I'm
good
with that (previous patch!) if you guys are as well.
Regards,
>
> John
>
Thanks for your feedback!
jdl
_______________________________________________
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev
