On 03/29/2016 12:23 AM, Laine Stump wrote:
On 03/28/2016 10:42 AM, Laine Stump wrote:
On 03/28/2016 06:43 AM, Shanzhi Yu wrote:
in commit 370608b, bitmap of in-used macvtap devices was introduced.
if there is already macvtap device created not by libvirt,
virNetDevMacVLanCreateWithVPortProfile will failed after try
MACVLAN_MAX_ID times call virNetDevMacVLanReleaseID
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1321546
NACK.
This is a bonafide bug, but not the right fix. Your patch would go
back and pick up the unused "macvtap0" name rather than trying
macvtap2, but once macvtap0 was in use, the next call (for yet another
macvtap device) would attempt to use macvtap2 (since macvtap0 and
macvtap1 were in use) and would then end up looping in the same manner
as it did without your patch (MACVLAN_MAX_ID times == 8191) and then
still reporting a failure.
Sigh. The above is incorrect, as I was trying to trace through the code
in my mind after mentally applying the patch, rather than actually
trying it.
Thanks for your explanations, actually your fix is really what I want, I
should go through virNetDevMacVLanReserveID carefully.
As a matter of fact, the reality is worse than the above description.
With this patch the device name is constructed by using the id number
that we *want* to reserve rather than the one that we actually do
reserve, but the bug in virNetDevMacVLanReserveID() is still there, so
in fact what will happen is that the newly patched loop will
1) format the name to use as "macvtap0"
2) ask for macvtap0
3) get back macvtap2, but ignore that
4) try/fail to create macvtap0
5) *NOT* release the id that it just allocated (which was macvtap2)
6) loop again
7) format "macvtap2"
8) ask for macvtap2
9) get back macvtap3, but ignore that
10) try/fail to create macvtap2
11) *NOT release the id that was just reserved (macvtap3)
12) loop again
13) format macvtap3
14) ask for macvtap3
15) get back macvtap4 (even though nobody is using macvtap3,
we reserved it in the last iteration, didn't use it,
and didn't free it!)
16) try/succeed to create macvtap3
So at this point, we have reserved macvtap2, macvtap3, and macvtap4,
even though libvirt is only using macvtap3 (and something outside of
libvirt is using macvtap2). The next time someone wants a macvtap
device, we'll again start out at 0, get back 5, try/fail to create 0,
ask for 1, get back 6, try/fail to create 1, ask for 2, get back 7,
try/fail to create 2, ask for 3, get back 8, try/fail to create 3, ask
for 4, get back 9, and finally succeed with macvtap4. So *now* we've
reserved all names up through macvtap9, but are only using macvtap0,
macvtap1 (these from previous), macvtap3, and macvtap4. We'll continue
like this, reserving n-1 extra names for each new macvtap device "n",
thus running out much more quickly.
Anyway, that's probably much more analysis than necessary. The main
points (in addition to the fact that it doesn't fix the root cause) are:
a) it creates the macvtap name based on the desired id# rather than the
one that was actually reserved
b) it never frees the id's that it ends up not using because the create
failed.
The proper fix is to call virBitmapNextClearBit() (inside
virNetDevMacVLanReserveID()) with the most recently failed id (or -1)
rather than with 0. This is what I've done in the following patch:
https://www.redhat.com/archives/libvir-list/2016-March/msg01286.html
Signed-off-by: Shanzhi Yu <shyu@xxxxxxxxxx>
---
src/util/virnetdevmacvlan.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 2409113..973363e 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -988,7 +988,8 @@ virNetDevMacVLanCreateWithVPortProfile(const char
*ifnameRequested,
MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
const char *pattern = (flags &
VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
- int rc, reservedID = -1;
+ int rc = -1;
+ int reservedID = 0;
char ifname[IFNAMSIZ];
int retries, do_retry = 0;
uint32_t macvtapMode;
@@ -1072,16 +1073,17 @@ virNetDevMacVLanCreateWithVPortProfile(const
char *ifnameRequested,
retries = MACVLAN_MAX_ID;
while (!ifnameCreated && retries) {
virMutexLock(&virNetDevMacVLanCreateMutex);
- reservedID = virNetDevMacVLanReserveID(reservedID, flags,
false, true);
- if (reservedID < 0) {
- virMutexUnlock(&virNetDevMacVLanCreateMutex);
- return -1;
- }
snprintf(ifname, sizeof(ifname), pattern, reservedID);
rc = virNetDevMacVLanCreate(ifname, type, macaddress, linkdev,
macvtapMode, &do_retry);
if (rc < 0) {
- virNetDevMacVLanReleaseID(reservedID, flags);
+ reservedID++;
+ reservedID = virNetDevMacVLanReserveID(reservedID,
flags, false, true);
+ if (reservedID < 0) {
+ virMutexUnlock(&virNetDevMacVLanCreateMutex);
+ return -1;
+ }
+
virMutexUnlock(&virNetDevMacVLanCreateMutex);
if (!do_retry)
return -1;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
Regards
shyu
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list