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

[Bruce Richardson]

On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Monday, September 18, 2017 11:32 AM
> > To: Thomas Monjalon <thomas at monjalon.net>
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; stephen at networkplumber.org; dev at dpdk.org; Shahaf Shuler
> > <shahafs at mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
> > 
> > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote:
> > > 13/09/2017 23:42, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > > 13/09/2017 14:56, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > > Konstantin, I would like your opinion about the proposal below.
> > > > > It is about making on the fly configuration more generic.
> > > > > You say it is possible to configure VLAN on the fly,
> > > > > and I think we should make it possible for other offload features.
> > > >
> > > > It would be a good thing, but I don't think it is possible for all offloads.
> > > > For some of them you still have to stop the queue(port) first.
> > > >
> > > > Also I am not sure what exactly do you propose?
> > > > Is that something like that:
> > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf)
> > > > - Instead of uint64_t offloads inside both  rte_eth_rxmode and te_eth_rxconf
> > > >   Introduce new functions:
> > > >
> > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask);
> > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask);
> > Would be useful to have a valid mask here, to indicate what bits to use.
> > That way, you can adjust one bit without worrying about what other bits
> > you may change in the process. There are probably apps out there that
> > just want to toggle a single bit on, and off, at runtime while ignoring
> > others.
> > Alternatively, we can have set/unset functions which enable/disable
> > offloads, based on the mask.
> 
> My thought was  that people would do:
> 
> uint64_t offload = rte_eth_get_port_rx_offload(port);
> offload |= RX_OFFLOAD_X;
> offload &= ~RX_OFFLOAD_Y;
> rte_eth_set_port_rx_offload(port, offload);
> 
> In that case, I think we don't really need a mask.
> 
Sure, that can work, I'm not concerned either way.

Overall, I think my slight preference would be to have set/unset,
enable/disable functions to make it clear what is happening, rather than
having to worry about the complete set each time.

uint64_t rte_eth_port_rx_offload_enable(port_id, offload_mask)
uint64_t rte_eth_port_rx_offload_disable(port_id, offload_mask)

each returning the bits failing (or bits changed if you like, but I prefer
bits failing as return value, since it means 0 == no_error).

/Bruce