Re: [PATCH mlx5-next 1/2] net/mlx5: Expose raw packet pacing APIs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, 2020-02-23 at 10:53 +0200, Yishai Hadas wrote:
> On 2/21/2020 9:04 PM, Saeed Mahameed wrote:
> > On Wed, 2020-02-19 at 21:05 +0200, Leon Romanovsky wrote:
> > > From: Yishai Hadas <yishaih@xxxxxxxxxxxx>
> > > 
> > > Expose raw packet pacing APIs to be used by DEVX based
> > > applications.
> > > The existing code was refactored to have a single flow with the
> > > new
> > > raw
> > > APIs.
> > > 
> > > The new raw APIs considered the input of 'pp_rate_limit_context',
> > > uid,
> > > 'dedicated', upon looking for an existing entry.
> > > 
> > > This raw mode enables future device specification data in the raw
> > > context without changing the existing logic and code.
> > > 
> > > The ability to ask for a dedicated entry gives control for
> > > application
> > > to allocate entries according to its needs.
> > > 
> > > A dedicated entry may not be used by some other process and it
> > > also
> > > enables the process spreading its resources to some different
> > > entries
> > > for use different hardware resources as part of enforcing the
> > > rate.
> > > 
> > 
> > It sounds like the dedicated means "no sharing" which means you
> > don't
> > need to use the mlx5_core API and you can go directly to FW.. The
> > problem is that the entry indices are managed by driver, and i
> > guess
> > this is the reason why you had to expand the mlx5_core API..
> > 
> 
> The main reason for introducing the new mlx5_core APIs was the need
> to 
> support the "shared mode" in a "raw data" format to prevent future 
> touching the kernel once PRM will support extra fields.
> As the RL indices are managed by the driver (mlx5_core) including
> the 
> sharing, we couldn’t go directly to FW, the legacy API was
> refactored 
> inside the core to have one flow with the new raw APIs.
> So we may need those APIs regardless the dedicated mode.
> 

I not a fan of legacy APIs, all of the APIs are mlx5 internals and i
would like to keep one API which is only PRM dependent as much as
possible.

Anyway thanks for the clarification, i think the patch is good as is,
we can improve and remove the legacy API in the future and keep the raw
API.

> 
> > I would like to suggest some alternatives to simplify the approach
> > and
> > allow using RAW PRM for DEVX properly.
> > 
> > 1. preserve RL entries for DEVX and let DEVX access FW directly
> > with
> > PRM commands.
> > 2. keep mlx5_core API simple and instead of adding this raw/non raw
> > api
> > and complicating the RL API with this dedicated bit:
> > 
> > just add mlx5_rl_{alloc/free}_index(), this will dedicate for you
> > the
> > RL index form the end of the RL indices database and you are free
> > to
> > access the FW with this index the way you like via direct PRM
> > commands.
> > 
> As mentioned above, we may still need the new mlx5_core raw APIs for
> the 
> shared mode which is the main usage of the API, we found it
> reasonable 
> to have the dedicate flag in the new raw alloc API instead of
> exposing 
> more two new APIs only for that.
> 
> Please note that even if we'll go with those 2 extra APIs for the 
> dedicated mode, we may still need to maintain in the core this 
> information to prevent returning this entry for other cases.
> 
> Also the idea to preserve some entries at the end might be wasteful
> as 
> there is no guarantee that DEVX will really be used, and even so it
> may 
> not ask for entries in a dedicated mode.
> 
> Presering them for this optional use case might prevent using them
> for 
> all other cases.
> 
> 
> > > The counter per entry mas changed to be u64 to prevent any option
> > > to
> >                     typo ^^^ was
> 
> Sure, thanks.
> 

Leon, Other than the typo i am good with this patch.
you can fix up the patch prior to pulling into mlx5-next, no need for
v2.

Acked-by: Saeed Mahameed <saeedm@xxxxxxxxxxxx> 


thanks,
Saeed.





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux