On Tue, Apr 03, 2018 at 03:45:18PM -0600, Jason Gunthorpe wrote:
> On Sun, Apr 01, 2018 at 03:08:06PM +0300, Leon Romanovsky wrote:
>
> > +int mlx5_cmd_alloc_memic(struct mlx5_memic *memic, phys_addr_t *addr,
> > + u64 length, u32 alignment)
> > +{
> > + struct mlx5_core_dev *dev = memic->dev;
> > + u64 num_memic_hw_pages = MLX5_CAP_DEV_MEM(dev, memic_bar_size)
> > + >> PAGE_SHIFT;
> > + u64 hw_start_addr = MLX5_CAP64_DEV_MEM(dev, memic_bar_start_addr);
> > + u32 max_alignment = MLX5_CAP_DEV_MEM(dev, log_max_memic_addr_alignment);
> > + u32 num_pages = DIV_ROUND_UP(length, PAGE_SIZE);
> > + u32 out[MLX5_ST_SZ_DW(alloc_memic_out)] = {};
> > + u32 in[MLX5_ST_SZ_DW(alloc_memic_in)] = {};
> > + u32 mlx5_alignment;
> > + u64 page_idx = 0;
> > + int ret = 0;
> > +
> > + if (!length || (length & MLX5_MEMIC_ALLOC_SIZE_MASK))
> > + return -EINVAL;
> > +
> > + /* mlx5 device sets alignment as 64*2^driver_value
> > + * so normalizing is needed.
> > + */
> > + mlx5_alignment = (alignment < MLX5_MEMIC_BASE_ALIGN) ? 0 :
> > + alignment - MLX5_MEMIC_BASE_ALIGN;
> > + if (mlx5_alignment > max_alignment)
> > + return -EINVAL;
> > +
> > + MLX5_SET(alloc_memic_in, in, opcode, MLX5_CMD_OP_ALLOC_MEMIC);
> > + MLX5_SET(alloc_memic_in, in, range_size, num_pages * PAGE_SIZE);
> > + MLX5_SET(alloc_memic_in, in, memic_size, length);
> > + MLX5_SET(alloc_memic_in, in, log_memic_addr_alignment,
> > + mlx5_alignment);
> > +
> > + do {
> > + spin_lock(&memic->memic_lock);
> > + page_idx = bitmap_find_next_zero_area(memic->memic_alloc_pages,
> > + num_memic_hw_pages,
> > + page_idx,
> > + num_pages, 0);
> > +
> > + if (page_idx + num_pages <= num_memic_hw_pages)
> > + bitmap_set(memic->memic_alloc_pages,
> > + page_idx, num_pages);
> > + else
> > + ret = -ENOMEM;
>
> So every memic allocation takes an entire BAR page even if there is
> empty space in pages already assigned to the context?
Yes, the reason to it is the combination of FW, HW limitations (continuous layout,
alignment and location of those pages) and SW (simplification and protection) that
we (Ariel, Yishai and I) preferred to use such allocation scheme.
>
> Seems rather inefficient? Could be improved in followup patches.
>
> > + spin_unlock(&memic->memic_lock);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + MLX5_SET64(alloc_memic_in, in, range_start_addr,
> > + hw_start_addr + (page_idx * PAGE_SIZE));
> > +
> > + ret = mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
> > + if (ret) {
> > + spin_lock(&memic->memic_lock);
> > + bitmap_clear(memic->memic_alloc_pages,
> > + page_idx, num_pages);
> > + spin_unlock(&memic->memic_lock);
> > +
> > + if (ret == -EAGAIN) {
> > + page_idx++;
> > + continue;
> > + }
> > +
> > + return ret;
> > + }
> > +
> > + *addr = pci_resource_start(dev->pdev, 0) +
> > + MLX5_GET64(alloc_memic_out, out, memic_start_addr);
> > +
> > + return ret;
> > + } while (page_idx < num_memic_hw_pages);
> > +
> > + return ret;
>
> This return/while/return construct is a pretty muddled bit of logic.
> Should just be
>
> while (page_idx < num_memic_hw_pages) {
> ..
> return 0;
> };
> return -ENOMEM;
>
> Jason
Attachment:
signature.asc
Description: PGP signature
