[libvirt] [PATCHv2] pci: properly handle out-of-order SRIOV virtual functions

[Laine Stump]

On 11/07/2013 02:32 PM, Ján Tomko wrote:
> On 11/07/2013 12:10 PM, Laine Stump wrote:
>>       while ((entry = readdir(dir))) {
>>          if (STRPREFIX(entry->d_name, "virtfn")) {
>> -            virPCIDeviceAddress *config_addr = NULL;
>> +            char *tempName;
>>  
>> -            if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
>> -                virReportOOMError();
>> +            /* save these to sort into virtual_functions array */
>> +            if (VIR_STRDUP(tempName, entry->d_name) < 0)
>> +                goto error;
>> +            if (VIR_APPEND_ELEMENT(virtfnNames, nVirtfnNames, tempName) < 0) {
>> +                VIR_FREE(tempName);
>>                  goto error;
>>              }
>> +        }
>> +    }
> Wouldn't it be easier to just count the entries beginning with "virtfn" and
> then go from 0 to n?

Yes (although that has the problem you indicate below). Or just use
Martin's idea of checking for existence of links starting with virtfn0
and increasing by one until there's a failure. I'd just been so wrapped
up in micro-changing the existing code that I didn't look for a
different solution.


>  (This would need a special return value for
> virPCIGetDeviceAddressFromSysfsLink if the link does not exist, if the entries
> in the directory can truly be sparse, or a virtfn-poorly-chosen-name appears)
>
>>  
>> -            VIR_DEBUG("Number of virtual functions: %d",
>> -                      *num_virtual_functions);
>> +    /* pre-allocate because we will be filling in out of order */
>> +    if (nVirtfnNames && VIR_ALLOC_N(*virtual_functions, nVirtfnNames) < 0)
>> +        goto error;
>> +    *num_virtual_functions = nVirtfnNames;
>>  
>> -            if (virPCIGetDeviceAddressFromSysfsLink(device_link,
>> -                                                    &config_addr) !=
>> -                SRIOV_FOUND) {
>> -                VIR_WARN("Failed to get SRIOV function from device "
>> -                         "link '%s'", device_link);
>> -                VIR_FREE(device_link);
>> -                continue;
>> -            }
>> +    for (i = 0; i < nVirtfnNames; i++) {
>> +        virPCIDeviceAddress *config_addr = NULL;
>> +        unsigned int virtfnIndex;
>>  
>> -            if (VIR_REALLOC_N(*virtual_functions,
>> -                              *num_virtual_functions + 1) < 0) {
>> -                VIR_FREE(config_addr);
>> -                goto error;
>> -            }
>> +        if (virBuildPath(&device_link, sysfs_path, virtfnNames[i]) < 0) {
>> +            virReportOOMError();
>> +            goto error;
>> +        }
>> +
>> +        if (virStrToLong_ui(virtfnNames[i] + strlen(virtfnPrefix),
>> +                            0, 10, &virtfnIndex) < 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Invalid virtual function link name '%s' "
>> +                             "in physical function directory '%s'"),
>> +                           virtfnNames[i], sysfs_path);
>> +            goto error;
>> +        }
> This rejects "virtfn-poor-name-choice" (which is fine by me, it's really a
> poor choice).

yeah, good point. Yet another reason this is a bad idea.


>
>> +
>> +        if (virtfnIndex >= nVirtfnNames) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Virtual function link name '%s' larger than "
>> +                             "total count of virtual functions %zu "
>> +                             "in physical function directory '%s'"),
>> +                           virtfnNames[i], nVirtfnNames, sysfs_path);
>> +            goto error;
>> +        }
>> +
>> +        if ((*virtual_functions)[virtfnIndex]) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Virtual function link name '%s' "
>> +                             "generates duplicate index %zu "
>> +                             "in physical function directory '%s'"),
>> +                           virtfnNames[i], nVirtfnNames, sysfs_path);
>> +            goto error;
>> +        }
> Can there really be two entries in a directory with the same name?

No, but conceivably there could be, e.g., "virtfn01" and "virtfn1". It
would indicate a serious problem in the kernel, but it's within the
realm of possibility. So this was basically a sanity check.


>
>>  
>> -            (*virtual_functions)[*num_virtual_functions] = config_addr;
>> -            (*num_virtual_functions)++;
>> +        if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) !=
>> +            SRIOV_FOUND) {
> This also ignores OOM errors.


yes, but in a non-dangerous manner. And there would eventually be an
error exit because the array would end up being sparse.


>> +            VIR_WARN("Failed to get SRIOV function from device link '%s'",
>> +                     device_link);
>>              VIR_FREE(device_link);
>> +            continue;
>>          }
>> +
>> +        VIR_DEBUG("Found virtual function %d", virtfnIndex);
>> +        (*virtual_functions)[virtfnIndex] = config_addr;
>> +        VIR_FREE(device_link);
>>      }
>
>
>> +    /* truncate any trailing nulls due to files having names starting
>> +     * with "virtfn" which weren't actually a link to a virtual
>> +     * function.
>> +     */
>> +    while (*num_virtual_functions &&
>> +           !(*virtual_functions)[(*num_virtual_functions) - 1])
>> +        (*num_virtual_functions)--;
>> +
>> +    /* failure of realloc when shrinking is safe */
>> +    ignore_value(VIR_REALLOC_N(*virtual_functions, *num_virtual_functions));
>> +
> This treats contiguous missing entries at the end differently than the missing
> ones at the beginning,

Exactly.


>  are the virtfns with larger indexes less important? :)

No. The logic behind this is that any extra entries at the end are due
to filenames that start with "virtfn" but aren't actually a link to a
virtual function at all (just a poorly chosen name for something else).
The actual virtual functions will all be virtfn[0-n] and contiguous. So
an empty entry at the end just indicates there were some items in the
directory that weren't virtual function links, while an empty entry in
the middle indicates that there was a problem resolving the link to an
actual virtual function.

Anyway, the fact that I need to explain this separately is more evidence
that I should just do it in the simpler manner that Martin suggested,
which I'm about to do now.


>> +    /* if there are any NULLs left in the array, fail and log an error
>> +     * - the virtual function array cannot be sparse.
>> +     */
>> +    for (i = 0; i < *num_virtual_functions; i++) {
>> +        if (!(*virtual_functions)[*num_virtual_functions]) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Missing virtual function link for VF %zu "
>> +                             "in physical function directory '%s'"),
>> +                           i, sysfs_path);
> In other words, virPCIGetDeviceAddressFromSysfsLink failed. If we really need
> to ignore entries that aren't a symlink with a PCI address, the array has to
> be sparse and we need to fix all the users :(

No, I believe the kernel will only create a contiguous array of virtual
functions, so we don't need to support a sparse array.