[dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework

[Shreyansh Jain]

On Monday 11 December 2017 08:08 PM, Gaëtan Rivet wrote:
> On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote:
>> On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote:
>>> On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
>>>> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
>>
>> [...]
>>
>>>>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>>>>> index 331d954..bd3c28e 100644
>>>>> --- a/lib/librte_eal/common/include/rte_bus.h
>>>>> +++ b/lib/librte_eal/common/include/rte_bus.h
>>>>> @@ -183,6 +183,51 @@ struct rte_bus_conf {
>>>>>     	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
>>>>>     };
>>>>> +/**
>>>>> + * Bus configuration items.
>>>>> + */
>>>>> +enum rte_bus_ctrl_item {
>>>>> +	RTE_BUS_CTRL_PROBE_MODE = 0,
>>>>> +	RTE_BUS_CTRL_ITEM_MAX,
>>>>> +};
>>>>
>>>> I am assuming that a driver implementation can take more than ITEM_MAX
>>>> control knobs. It is opaque to the library. Are we on same page?
>>>>
>>>> For example, a bus driver can implement:
>>>>
>>>> rte_bus_XXX_ctrl_item {
>>>> 	<Leaving space for allowing rte_bus.h implementations>
>>>> 	RTE_BUS_XYZ_KNOB_1 = 100,
>>>> 	RTE_BUS_XYZ_KNOB_2,
>>>> 	RTE_BUS_XYZ_KNOB_3,
>>>> };
>>>>
>>>> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
>>>>
>>>> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
>>>> the control knob to RTE_BUS_CTRL_ITEM_MAX.
>>>> I hope that such restrictions would not float to library layer.
>>>>
>>>> If we are on same page, should this be documented as a code comment
>>>> somewhere?
>>>> if not, do you think what I am stating makes sense?
>>>>
>>>
>>> I see what you mean, but I'm not sure it would be a good thing.
>>> Actually, I think proposing this ITEM_MAX was a mistake.
>>>
>>> Regarding the specific bus knobs:
>>>
>>> - If a single bus needs this knob, then it would be better for the dev
>>>     to add it as part of the bus' public API, following the correct
>>>     library versioning processes. This would not break this bus control
>>>     structure ABI.
>>
>> Sorry, but can you elaborate on "...add it as part of bus' public API"?
>>
>> This is what I had in mind:
>>
>> ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);
>>
>> (unlike specific functions like probe_mode_get/set and iova_mode_get/set)
>>
>> Where ctrl_fn would then point to a method specific to bus for KNOB_1
>> configuration parameter.
>> Thereafter, ctrl_fn(KNOB_1, void *arg).
>>
>> What other public API method are you hinting at?
>>
>>
> 
> I was thinking that buses would simply expose a function
> 
> rte_busname_xyz_knob1(void *arg);

Yes, that is possible but only for cases where some very specific 
functionality needs to be exposed which is not expected to be 
generalized ever.

> 
> as part of their public API. This would not require an ABI break for
> this bus, as it would only be an API extension and would not use
> callbacks within the bus structure.

Yes, agree with your point.
As such APIs are outside control of DPDK framework, they are something 
which will never impact the library layer.

> 
> Thus, I think that for buses tempted to propose a specific API, this would be
> the cleanest way. >
> The bus proposing it as part of a custom control section would only be
> interesting when the operation is expected to become a standard API for
> other buses but was not yet accepted. Applications would be able to use
> the interface and the ITEM could be added later. But I doubt this is
> encouraging best practices as far as API evolution go.

So, technically both are feasible: 1) having a bus specific API like 
rte_busname_xyz_knob1 and 2) expanding OPS with bus specific values and 
allowing application to use them.

But, in either case, if the APIs can be generalized and/or can be used 
by multiple buses, they can definitely be moved into the library API 
(e.g. rte_bus_probe_mode_set) and/or can be added to rte_bus_ctrl_item.

To summarize, I am OK with your approach.

> 
>>>
>>> - If more than one bus implement this knob, then it should be proposed
>>>     as part of the library API. Buses adding this new knob would break
>>>     their ABI, other buses would be left untouched.
>>
>> Agree, if more than one bus implements same operation.
>>
>>>
>>> This makes me realize that proposing this ITEM_MAX value is not good to
>>> the intended purpose of this patchset:
>>>
>>> - If a bus implementation use a reference to ITEM_MAX, then the control
>>>     structure ABI would be broken by any new control knob added, even if the
>>>     bus does not implement it. Granted, it would not break the driver
>>>     structure itself, but still. My PCI implementation is thus incorrect.
>>
>> Changes to enum wouldn't break ABI as far as I understand. Adding a new
>> entry only expands it to a new declaration without impacting its size or
>> signature.
>>
>>>
>>> Therefore I think that it would be best to remove this ITEM_MAX altogether,
>>> forcing bus developpers to use other ways that would not break their
>>> ABIs every other release.
>>>
>>
>> Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. But,
>> not for the "ABI break" reason.
> 
> Adding the enum would not break ABI indeed, but I was thinking about the
> way the bus control structure would be declared.
> 
> However, upon second inspection on the my PCI implementation, I did not
> actually use ITEM_MAX:
> 
>> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
>> »      [RTE_BUS_CTRL_PROBE_MODE] = {
>> »      »       [RTE_BUS_CTRL_GET] = pci_probe_mode_get,
>> »      »       [RTE_BUS_CTRL_SET] = pci_probe_mode_set,
>> »      },
>> };
> 
> I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX.
> So my line of thought was simply that if any new item was declared, the control
> structure would then change size.
> 
> But I was mistaken, so that's actually not a problem :)
> 
> Having ITEM_MAX available would still make those kind of mistakes
> possible however. It might be better to prevent it completely by
> removing it. This would however also prevent a custom control section.

It might be a naive question, but, why do you think it would prevent a 
custom control section?

Assuming that only RTE_BUS_CTRL_ITEM_MAX is not available 
(RTE_BUS_CTRL_OPS_MAX is available), Bus driver can still define:

static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
	[ITEM_1] = {
		{...},
		{...},
	},
	[ITEM_2] = {
		{...},
		{...},
	},
	[ITEM_NOT_IN_RTE_BUS.H] = {
		{...},
		{...},
	},
}

(I know you disagree with third element of above definition - but 
somehow I feel it is a good addition for defining a knob which doesn't 
require an additional API call. Just assume as an example for now, please!)

> 
> Do you think this would be useful enough to justify the slightly more
> complex maintenance and review of bus implementations?
> 

Having RTE_BUS_CTRL_ITEM_MAX is helpful if one has to iterate over all 
ctrl_items - but, that might never be required in my perception. Other 
than that, I don't have a strong reason to say that ITEM_MAX is required.
Though, same is not true for OP_MAX - which is required for definitions 
like above.