netdev - Re: [PATCH v10 vfio 1/7] vfio: Commonize combine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50d470c5-360e-c350-ff8a-db93d70ac810@amd.com>
Date: Fri, 16 Jun 2023 11:37:37 -0700
From: Brett Creeley <bcreeley@....com>
To: "Tian, Kevin" <kevin.tian@...el.com>,
 Brett Creeley <brett.creeley@....com>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
 "jgg@...dia.com" <jgg@...dia.com>, "yishaih@...dia.com"
 <yishaih@...dia.com>,
 "shameerali.kolothum.thodi@...wei.com" <shameerali.kolothum.thodi@...wei.com>
Cc: "shannon.nelson@....com" <shannon.nelson@....com>
Subject: Re: [PATCH v10 vfio 1/7] vfio: Commonize combine_ranges for use in
 other VFIO drivers

On 6/15/2023 11:52 PM, Tian, Kevin wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
>> From: Brett Creeley <brett.creeley@....com>
>> Sent: Saturday, June 3, 2023 6:03 AM
>>
>> +void vfio_combine_iova_ranges(struct rb_root_cached *root, u32
>> cur_nodes,
>> +                           u32 req_nodes)
>> +{
>> +     struct interval_tree_node *prev, *curr, *comb_start, *comb_end;
>> +     unsigned long min_gap, curr_gap;
>> +
>> +     /* Special shortcut when a single range is required */
>> +     if (req_nodes == 1) {
>> +             unsigned long last;
>> +
>> +             comb_start = interval_tree_iter_first(root, 0, ULONG_MAX);
>> +             curr = comb_start;
>> +             while (curr) {
>> +                     last = curr->last;
>> +                     prev = curr;
>> +                     curr = interval_tree_iter_next(curr, 0, ULONG_MAX);
>> +                     if (prev != comb_start)
>> +                             interval_tree_remove(prev, root);
>> +             }
>> +             comb_start->last = last;
>> +             return;
>> +     }
>> +
>> +     /* Combine ranges which have the smallest gap */
>> +     while (cur_nodes > req_nodes) {
>> +             prev = NULL;
>> +             min_gap = ULONG_MAX;
>> +             curr = interval_tree_iter_first(root, 0, ULONG_MAX);
>> +             while (curr) {
>> +                     if (prev) {
>> +                             curr_gap = curr->start - prev->last;
>> +                             if (curr_gap < min_gap) {
>> +                                     min_gap = curr_gap;
>> +                                     comb_start = prev;
>> +                                     comb_end = curr;
>> +                             }
>> +                     }
>> +                     prev = curr;
>> +                     curr = interval_tree_iter_next(curr, 0, ULONG_MAX);
>> +             }
>> +             comb_start->last = comb_end->last;
>> +             interval_tree_remove(comb_end, root);
>> +             cur_nodes--;
>> +     }
>> +}
>> +EXPORT_SYMBOL_GPL(vfio_combine_iova_ranges);
>> +
> 
> Being a public function please follow the kernel convention with comment
> explaining what this function actually does.

I've seen many cases that there's no documentation for public functions 
and I don't think any documentation is needed for this function as the 
name is self explanatory. VFIO drivers can use this to combine iova 
ranges, hence why I named it vfio_combine_iova_ranges().

> 
> btw while you rename it with 'vfio' and 'iova' keywords, the actual logic
> has nothing to do with either of them. Does it make more sense to move it
> to a more generic library?

I think it *could* go into a more generic library, but at this point in 
time I think it belongs here. As mentioned in the previous comment the 
function name describes its exact purpose. If/when it ever gets more 
users it can be moved and renamed.