diff mbox series

[v10,vfio,1/7] vfio: Commonize combine_ranges for use in other VFIO drivers

Message ID 20230602220318.15323-2-brett.creeley@amd.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series pds_vfio driver | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Brett Creeley June 2, 2023, 10:03 p.m. UTC 
Currently only Mellanox uses the combine_ranges function. The
new pds_vfio driver also needs this function. So, move it to
a common location for other vendor drivers to use.

Also, Simon Harmon noticed that RCT ordering was not followed
for vfio_combin_iova_ranges(), so fix that.

Cc: Yishai Hadas <yishaih@nvidia.com>
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/vfio/pci/mlx5/cmd.c | 48 +------------------------------------
 drivers/vfio/vfio_main.c    | 47 ++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h        |  3 +++
 3 files changed, 51 insertions(+), 47 deletions(-)

Comments

Tian, Kevin June 16, 2023, 6:52 a.m. UTC | #1 
> From: Brett Creeley <brett.creeley@amd.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.

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?
Brett Creeley June 16, 2023, 6:37 p.m. UTC | #2 
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@amd.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.
diff mbox series

Patch

diff --git a/drivers/vfio/pci/mlx5/cmd.c b/drivers/vfio/pci/mlx5/cmd.c
index deed156e6165..7f6c51992a15 100644
--- a/drivers/vfio/pci/mlx5/cmd.c
+++ b/drivers/vfio/pci/mlx5/cmd.c
@@ -732,52 +732,6 @@  void mlx5fv_cmd_clean_migf_resources(struct mlx5_vf_migration_file *migf)
 	mlx5vf_cmd_dealloc_pd(migf);
 }
 
-static void combine_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;
-	unsigned long curr_gap;
-
-	/* Special shortcut when a single range is required */
-	if (req_nodes == 1) {
-		unsigned long last;
-
-		curr = comb_start = interval_tree_iter_first(root, 0, ULONG_MAX);
-		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--;
-	}
-}
-
 static int mlx5vf_create_tracker(struct mlx5_core_dev *mdev,
 				 struct mlx5vf_pci_core_device *mvdev,
 				 struct rb_root_cached *ranges, u32 nnodes)
@@ -800,7 +754,7 @@  static int mlx5vf_create_tracker(struct mlx5_core_dev *mdev,
 	int i;
 
 	if (num_ranges > max_num_range) {
-		combine_ranges(ranges, nnodes, max_num_range);
+		vfio_combine_iova_ranges(ranges, nnodes, max_num_range);
 		num_ranges = max_num_range;
 	}
 
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index f0ca33b2e1df..3bde62f7e08b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -865,6 +865,53 @@  static int vfio_ioctl_device_feature_migration(struct vfio_device *device,
 	return 0;
 }
 
+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);
+
 /* Ranges should fit into a single kernel page */
 #define LOG_MAX_RANGES \
 	(PAGE_SIZE / sizeof(struct vfio_device_feature_dma_logging_range))
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 2c137ea94a3e..f49933b63ac3 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -245,6 +245,9 @@  int vfio_mig_get_next_state(struct vfio_device *device,
 			    enum vfio_device_mig_state new_fsm,
 			    enum vfio_device_mig_state *next_fsm);
 
+void vfio_combine_iova_ranges(struct rb_root_cached *root, u32 cur_nodes,
+			      u32 req_nodes);
+
 /*
  * External user API
  */