Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Sep 18, 2017 at 02:27:25PM +0000, Shahaf Shuler wrote:
> Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > From: Richardson, Bruce
> > > > > >
> > > > > > I think we all in favor to have a separate API here.
> > > > > > Though from the discussion we had at latest TB, I am not sure it
> > > > > > is doable in 17.11 timeframe.
> > > > >
> > > > > Ok, so does that imply no change in this release, and that the
> > > > > existing set is to be ignored?
> > > >
> > > > No, my understanding the current plan is to go forward with Shahaf
> > > > patches, and then apply another one (new set/get API) on top of them.
> > >
> > > Yes, it is what we agreed (hope to see it in minutes).
> > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > Bruce, do you want to make it a try?
> > 
> > If I have the chance, I can try, but given how short time is and that userspace
> > is on next week, I very much doubt I'll even get it started.
> 
> I wasn't aware to the techboard decision on the extra patchset needed.
> I think it will be wrong to introduce an API on 17.11 and change it again on 18.02.  
> I will do my best to make everything ready for 17.11 so we can have one solid API on top of which all PMDs and application will be converted. Considering some Holidays and the DPDK summit I won't have much time to work on it.
> 
> The plan is as follows:
> 1.  complete the last comment on the current series and integrate it.
> 2. send a new patchset to convert to the API suggested above.
> 
> Aggregating the different suggestions I come up with the below. if this is agreed, then I will move with the implementation.
> (I thought it is good to return error values for the get function).

I'd rather you didn't. :-) The only realistic error I would consider is
an invalid port id, and I think returning 0 - no offloads - is fine in
those cases. The user will pretty quickly discover it's an invalid port
id anyway, so I prefer a get function to just return the value as a
return value and be done with it!

Otherwise, these will do fine. I would prefer some way to only change
one offload at a time without having to call "get" and do bit twiddling
before a call to "set", but this will be ok, if others are happy with
it.

If we at least get the return value as the mask of enabled offloads we
can at least shorten some cases as e.g.
rte_eth_set_port_tx_offloads(port_id, rte_eth_get_port_tx_offloads(port_id) | OFFLOAD_X);

/Bruce

> 
> **                                                                            
> * Get Tx offloads set on a specific port.                                     
> *                                                                             
> * @param port_id                                                              
> *   The port identifier of the Ethernet device.                               
> * @param offloads                                                             
> *   A pointer to uint64_t where the offloads flags                            
> *   will be filled using DEV_TX_OFFLOAD_* flags.                              
> * @return                                                                     
> *   - (0) if successful.                                                      
> *   - (-ENOTSUP or -ENODEV) on failure.                                       
> */                                                                            
> int rte_eth_get_port_tx_offloads(uint8_t port_id, uint64_t *offloads);         
>                                                                               
> **                                                                            
> * Get Tx offloads set on a specific queue.                                    
> *                                                                             
> * @param port_id                                                              
> *   The port identifier of the Ethernet device.                               
> * @param queue_id                                                             
> *   The queue identifier.                                                     
> * @param offloads                                                             
> *   A pointer to uint64_t where the offloads flags                            
> *   will be filled using DEV_TX_OFFLOAD_* flags.                              
> * @return                                                                     
> *   - (0) if successful.                                                      
> *   - (-ENOTSUP or -ENODEV) on failure.                                       
> */                                                                            
> int rte_eth_get_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,          
>                                  uint64_t *offloads);                         
> **                                                                            
> * Set Tx offloads on a specific port.                                         
> *                                                                             
> * @param port_id                                                              
> *   The port identifier of the Ethernet device.                               
> * @param offloads_mask                                                        
> *   Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.          
> * @return                                                                     
> *   (0) if all offloads set successfully, otherwise offloads                  
> *   flags which were not set.                                                 
> *                                                                             
> */                                                                            
> uint64_t rte_eth_set_port_tx_offloads(uint8_t port_id, uint64_t offloads_mask);
> 
> /**                                                                       
>  * Set Tx offloads on a specific queue.                                   
>  *                                                                        
>  * @param port_id                                                         
>  *   The port identifier of the Ethernet device.                          
>  * @param queue_id                                                        
>  *   The queue identifier.                                                
>  * @param offloads_mask                                                   
>  *   Indicates which offloads to be set using DEV_TX_OFFLOAD_* flags.     
>  * @return                                                                
>  *   (0) if all offloads set successfully, otherwise offloads             
>  *   flags which were not set.                                            
>  *                                                                        
>  */                                                                       
> uint64_t rte_eth_set_queue_tx_offloads(uint8_t port_id, uint16_t queue_id,
>                                        uint64_t offloads_mask);           
> /**                                                                       
>  * Get Rx offloads set on a specific port.                                
>  *                                                                        
>  * @param port_id                                                         
>  *   The port identifier of the Ethernet device.                          
>  * @param offloads                                                        
>  *   A pointer to uint64_t where the offloads flags                       
>  *   will be filled using DEV_RX_OFFLOAD_* flags.                         
>  * @return                                                                
>  *   - (0) if successful.                                                 
>  *   - (-ENOTSUP or -ENODEV) on failure.                                  
>  */                                                                       
> int rte_eth_get_port_rx_offloads(uint8_t port_id, uint64_t *offloads);    
>                                                                           
> /**                                                                       
>  * Get Rx offloads set on a specific queue.                               
>  *                                                                        
>  * @param port_id                                                         
>  *   The port identifier of the Ethernet device.                          
>  * @param queue_id                                                        
>  *   The queue identifier.                                                
>  * @param offloads                                                        
>  *   A pointer to uint64_t where the offloads flags                       
>  *   will be filled using DEV_RX_OFFLOAD_* flags.                         
>  * @return                                                                
>  *   - (0) if successful.                                                 
>  *   - (-ENOTSUP or -ENODEV) on failure.                                  
>  */                                                                       
> int rte_eth_get_queue_rx_offlaods(uint8_t port_id, uint16_t queue_id,     
>                                   uint64_t *offloads);   
> 
> /**                                                                            
>  * Set Rx offloads on a specific port.                                         
>  *                                                                             
>  * @param port_id                                                              
>  *   The port identifier of the Ethernet device.                               
>  * @param offloads_mask                                                        
>  *   Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.          
>  * @return                                                                     
>  *   (0) if all offloads set successfully, otherwise offloads                  
>  *   flags which were not set.                                                 
>  *                                                                             
>  */                                                                            
> uint64_t rte_eth_set_port_rx_offloads(uint8_t port_id, uint64_t offloads_mask);
>                                                                                
> /**                                                                            
>  * Set Rx offloads on a specific port.                                         
>  *                                                                             
>  * @param port_id                                                              
>  *   The port identifier of the Ethernet device.                               
>  * @param queue_id                                                             
>  *   The queue identifier.                                                     
>  * @param offloads_mask                                                        
>  *   Indicates which offloads to be set using DEV_RX_OFFLOAD_* flags.          
>  * @return                                                                     
>  *   (0) if all offloads set successfully, otherwise offloads                  
>  *   flags which were not set.                                                 
>  *                                                                             
>  */                                                                            
> uint64_t rte_eth_set_queue_rx_offloads(uint8_t port_id, uint16_t queue_id,     
>                                        uint64_t offloads_mask);                                 
> 
> > 
> > /Bruce