Re: [RFC] net: store port/representative id in metadata_dst

[John Fastabend <john.fastabend@gmail.com>]

On 16-09-23 01:17 PM, Jakub Kicinski wrote:
> On Fri, 23 Sep 2016 10:22:59 -0700, Samudrala, Sridhar wrote:
>> On 9/23/2016 8:29 AM, Jakub Kicinski wrote:
>>> On Fri, 23 Sep 2016 07:23:26 -0700, John Fastabend wrote:  
>>>> Yep, I like the idea in general. I had a slightly different approach in
>>>> mind though. If you look at __dev_queue_xmit() there is a void
>>>> accel_priv pointer (gather you found this based on your commit note).
>>>> My take was we could extend this a bit so it can be used by the VFR
>>>> devices and they could do a dev_queue_xmit_accel(). In this way there is
>>>> no need to touch /net/core/{filter, dst, ip_tunnel}.c etc. Maybe the
>>>> accel logic needs to be extended to push the priv pointer all the way
>>>> through the xmit routine of the target netdev though. This should look
>>>> a lot like the macvlan accelerated xmit device path without the
>>>> switching logic.
>>>>
>>>> Of course maybe the name would be extended to dev_queue_xmit_extended()
>>>> or something.
>>>>
>>>> So the flow on ingress would be,
>>>>
>>>>    1. pkt_received_by_PF_netdev
>>>>    2. PF_netdev reads some tag off packet/descriptor and sets correct
>>>>       skb->dev field. This is needed so stack "sees" packets from
>>>>       correct VF ports.
>>>>    3. packet passed up to stack.
>>>>
>>>> I guess it is a bit "zombie" like on the receive path because the packet
>>>> is never actually handled by VF netdev code per se and on egress can
>>>> traverse both the VFR and PF netdevs qdiscs. But on the other hand the
>>>> VFR netdevs and PF netdevs are all in the same driver. Plus using a
>>>> queue per VFR is a bit of a waste as its not needed and also hardware
>>>> may not have any mechanism to push VF traffic onto a rx queue.
>>>>
>>>> On egress,
>>>>
>>>>    1. VFR xmit is called
>>>>    2. VFR xmit calls dev_queue_xmit_accel() with some meta-data if needed
>>>>       for the lower netdev
>>>>    3. lower netdev sends out the packet.
>>>>
>>>> Again we don't need to waste any queues for each VFR and the VFR can be
>>>> a LLTX device. In this scheme I think you avoid much of the changes in
>>>> your patch and keep it all contained in the driver. Any thoughts?  
>>
>> The 'accel' parameter in dev_queue_xmit_accel() is currently only passed
>> to ndo_select_queue() via netdev_pick_tx() and is used to select the tx 
>> queue.
>> Also, it is not passed all the way to the driver specific xmit routine.  
>> Doesn't it require
>> changing all the driver xmit routines if we want to pass this parameter?
>>
>>> Goes without saying that you have a much better understanding of packet
>>> scheduling so please bear with me :)  My target model is that I have
>>> n_cpus x "n_tc/prio" queues on the PF and I want to transmit the
>>> fallback traffic over those same queues.  So no new HW queues are used
>>> for VFRs at all.  This is a reverse of macvlan offload which AFAICT has
>>> "bastard hw queues" which actually TX for a separate software device.
>>>
>>> My understanding was that I can rework this model to have software
>>> queues for VFRs (#sw queues == #PF queues + #VFRs) but no extra HW
>>> queues (#hw queues == #PF queues) but then when the driver sees a
>>> packet on sw-only VFR queue it has to pick one of the PF queues (which
>>> one?), lock PF software queue to own it, and only then can it
>>> transmit.  With the dst_metadata there is no need for extra locking or
>>> queue selection.  
>>
>> Yes.  The VFPR netdevs don't have any HW queues associated with them and 
>> we would like
>> to use the PF queues for the xmit.
>> I was also looking into some way of passing the port id via skb 
>> parameter to the
>> dev_queue_xmit() call so that the PF xmit routine can do a directed 
>> transmit to a specifc VF.
>> Is skb->cb an option to pass this info?
>> dst_metadata approach would work  too if it is acceptable.
> 
> I don't think we can trust skb->cb to be set to anything meaningful
> when the skb is received by the lower device. 
> 

Agreed. I wouldn't recommend using skb->cb. How about passing it through
dev_queue_xmit_accel() through to the driver?

If you pass the metadata through the dev_queue_xmit_accel() handle tx
queue  selection would work using normal mechanisms (xps, select_queue,
cls  hook, etc.). If you wanted to pick some specific queue based on
policy the policy could be loaded into one of those hooks.

.John