[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic |
Date: |
Wed, 17 Apr 2013 18:48:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 |
Il 17/04/2013 17:59, Michael R. Hines ha scritto:
>>> Failure already happens for unknown capabilities.
>> Maybe I misread the code, but all I saw is:
>>
>> + if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
>> + rdma->chunk_register_destination = true;
>> + } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
>> + /* handle new capability */
>> + }
>>
>> and no failure. I may well be wrong of course.
>
> Sorry I miscommunicated. By "failure" I meant "disagreement".
> My design allows the two sides to "agree" with each other as I
> included in the documentation like this:
>
> 1. User sets capability on source (or not)
>
> 2. If 'true', source transmits our "intent" to activate capability to
> destination
>
> If (destination knows about the capability)
> then, enable on both sides
> else
> disable on both sides
>
> 3. If 'false', destination will never see the flag and will never know
Great. That was my understanding too.
>>>> + goto err_rdma_server_wait;
>>>> + }
>>>> +
>>>> + if (cap.version == RDMA_CONTROL_VERSION_1) {
>>>> + if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
>>>> + rdma->chunk_register_destination = true;
>>>> + } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
>>>> + /* handle new capability */
>>>> + }
>>>> As mentioned above, please drop this "else if". But in general, this
>>>> "if" is useless. Please replace it with an
>>>>
>>>> /* We only support one version, and we rejected all
>>>> * others above.
>>>> */
>>>> assert(cap.version == RDMA_CONTROL_VERSION_CURRENT);
>>> We don't want to kill QEMU with an assertion, do we?
>>> Shouldn't we throw the error back to the user?
>> You already filtered versions < min and > current with a decent error,
>> so the only reamining version at this point is 1. You know cap.version
>> == RDMA_CONTROL_VERSION_CURRENT ("we rejected all others above").
>
> So, as I described above, when version 2 comes out, both QEMU
> instances are able to "agree" only on the minimum capabilities
> that both sides support and still allow the migration to begin.
>
> For that reason, I don't think we should be doing assert() here.
> We want this to be compatible across multiple QEMU versions.
The point of the assert is to ensure that all versions are either
handled correctly, or rejected. It's to distinguish bugs from
unsupported versions.
switch (cap.version) {
case RDMA_CONTROL_VERSION_CURRENT:
...;
break;
default:
assert(cap.version < RDMA_CONTROL_VERSION_MIN ||
cap.version > RDMA_CONTROL_VERSION_CURRENT);
/* give error */
}
Then you can simply add new cases when new versions come out.
Paolo
- [Qemu-devel] [PULL 6/8] rdma: send pc.ram, (continued)
- [Qemu-devel] [PULL 6/8] rdma: send pc.ram, mrhines, 2013/04/15
- [Qemu-devel] [PULL 8/8] rdma: add documentation, mrhines, 2013/04/15
- [Qemu-devel] [PULL 5/8] rdma: core rdma logic, mrhines, 2013/04/15
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Paolo Bonzini, 2013/04/16
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Paolo Bonzini, 2013/04/16
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Michael R. Hines, 2013/04/16
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Michael R. Hines, 2013/04/16
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Paolo Bonzini, 2013/04/17
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic, Michael R. Hines, 2013/04/17
- Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic,
Paolo Bonzini <=
Re: [Qemu-devel] [PULL 0/8] rdma: migration support, Paolo Bonzini, 2013/04/15