| Message ID | 20201118232014.2910642-1-awogbemila@google.com |
|---|---|
| Headers | show |
| Series | GVE Raw Addressing | expand |
On Wed, 18 Nov 2020 15:20:10 -0800 David Awogbemila wrote: > Patch 1: Use u8 instead of bool for raw_addressing bit in gve_priv structure. > Simplify pointer arithmetic: use (option + 1) in gve_get_next_option. > Separate option parsing switch statement into individual function. > Patch 2: Use u8 instead of bool for raw_addressing bit in gve_gve_rx_data_queue structure. > Correct typo in gve_desc.h comment (s/than/then/). > Change gve_rx_data_slot from struct to union. > Remove dma_mapping_error path change in gve_alloc_page - it should > probably be a bug fix. > Use & to obtain page address from data_ring->addr. > Move declarations of local variables i and slots to if statement where they > are used within gve_rx_unfill_pages. > Simplify alloc_err path by using "while(i--)", eliminating need for extra "int j" > variable in gve_prefill_rx_pages. > Apply byteswap to constant in gve_rx_flip_buff. > Remove gve_rx_raw_addressing as it does not do much more than gve_rx_add_frags. > Remove stats update from elseif block, no need to optimize for infrequent case of > work_done = 0. > Patch 3: Use u8 instead of bool for can_flip in gve_rx_slot_page_info. > Move comment in gve_rx_flip_buff to earlier, more relevant patch. > Fix comment wrap in gve_rx_can_flip_buffers. > Use ternary statement for gve_rx_can_flip_buffers. > Correct comment in gve_rx_qpl. > Patch 4: Use u8 instead of bool in gve_tx_ring structure. > Get rid of unnecessary local variable "dma" in gve_dma_sync_for_device. You need to start CCing people who gave you feedback, and discuss the feedback _before_ you send another version of the patchset. CCing Alex and Saeed
Where is the description of what this patch set is meant to do? I don't recall if I reviewed that in the last patch set but usually the cover page should tell us something about the patch set and not just be a list of changes which I assume are diffs from v3? On Wed, Nov 18, 2020 at 3:22 PM David Awogbemila <awogbemila@google.com> wrote: > > Patch 1: Use u8 instead of bool for raw_addressing bit in gve_priv structure. > Simplify pointer arithmetic: use (option + 1) in gve_get_next_option. > Separate option parsing switch statement into individual function. > Patch 2: Use u8 instead of bool for raw_addressing bit in gve_gve_rx_data_queue structure. > Correct typo in gve_desc.h comment (s/than/then/). > Change gve_rx_data_slot from struct to union. > Remove dma_mapping_error path change in gve_alloc_page - it should > probably be a bug fix. > Use & to obtain page address from data_ring->addr. > Move declarations of local variables i and slots to if statement where they > are used within gve_rx_unfill_pages. > Simplify alloc_err path by using "while(i--)", eliminating need for extra "int j" > variable in gve_prefill_rx_pages. > Apply byteswap to constant in gve_rx_flip_buff. > Remove gve_rx_raw_addressing as it does not do much more than gve_rx_add_frags. > Remove stats update from elseif block, no need to optimize for infrequent case of > work_done = 0. > Patch 3: Use u8 instead of bool for can_flip in gve_rx_slot_page_info. > Move comment in gve_rx_flip_buff to earlier, more relevant patch. > Fix comment wrap in gve_rx_can_flip_buffers. > Use ternary statement for gve_rx_can_flip_buffers. > Correct comment in gve_rx_qpl. > Patch 4: Use u8 instead of bool in gve_tx_ring structure. > Get rid of unnecessary local variable "dma" in gve_dma_sync_for_device. > > Catherine Sullivan (3): > gve: Add support for raw addressing device option > gve: Add support for raw addressing to the rx path > gve: Add support for raw addressing in the tx path > > David Awogbemila (1): > gve: Rx Buffer Recycling > > drivers/net/ethernet/google/gve/gve.h | 38 +- > drivers/net/ethernet/google/gve/gve_adminq.c | 90 ++++- > drivers/net/ethernet/google/gve/gve_adminq.h | 15 +- > drivers/net/ethernet/google/gve/gve_desc.h | 19 +- > drivers/net/ethernet/google/gve/gve_main.c | 11 +- > drivers/net/ethernet/google/gve/gve_rx.c | 403 ++++++++++++++----- > drivers/net/ethernet/google/gve/gve_tx.c | 211 ++++++++-- > 7 files changed, 620 insertions(+), 167 deletions(-) > > -- > 2.29.2.299.gdc1121823c-goog >
On Wed, 2020-11-18 at 15:20 -0800, David Awogbemila wrote: > From: Catherine Sullivan <csully@google.com> > > Add support to describe device for parsing device options. As > the first device option, add raw addressing. > > "Raw Addressing" mode (as opposed to the current "qpl" mode) is an > operational mode which allows the driver avoid bounce buffer copies > which it currently performs using pre-allocated qpls > (queue_page_lists) > when sending and receiving packets. > For egress packets, the provided skb data addresses will be > dma_map'ed and > passed to the device, allowing the NIC can perform DMA directly - the > driver will not have to copy the buffer content into pre-allocated > buffers/qpls (as in qpl mode). > For ingress packets, copies are also eliminated as buffers are handed > to > the networking stack and then recycled or re-allocated as > necessary, avoiding the use of skb_copy_to_linear_data(). > > This patch only introduces the option to the driver. > Subsequent patches will add the ingress and egress functionality. > > Reviewed-by: Yangchun Fu <yangchun@google.com> > Signed-off-by: Catherine Sullivan <csully@google.com> > Signed-off-by: David Awogbemila <awogbemila@google.com> > --- > ... > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c > b/drivers/net/ethernet/google/gve/gve_adminq.c > index 24ae6a28a806..1e2d407cb9d2 100644 > --- a/drivers/net/ethernet/google/gve/gve_adminq.c > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c > @@ -14,6 +14,57 @@ > #define GVE_ADMINQ_SLEEP_LEN 20 > #define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK 100 > > +#define GVE_DEVICE_OPTION_ERROR_FMT "%s option error:\n" \ > +"Expected: length=%d, feature_mask=%x.\n" \ > +"Actual: length=%d, feature_mask=%x.\n" > + > +static inline > +struct gve_device_option *gve_get_next_option(struct > Following Dave's policy, no static inline functions in C files. This is control path so you don't really need the inline here.
On Thu, Nov 19, 2020 at 12:21 PM Saeed Mahameed <saeed@kernel.org> wrote: > > On Wed, 2020-11-18 at 15:20 -0800, David Awogbemila wrote: > > From: Catherine Sullivan <csully@google.com> > > > > Add support to describe device for parsing device options. As > > the first device option, add raw addressing. > > > > "Raw Addressing" mode (as opposed to the current "qpl" mode) is an > > operational mode which allows the driver avoid bounce buffer copies > > which it currently performs using pre-allocated qpls > > (queue_page_lists) > > when sending and receiving packets. > > For egress packets, the provided skb data addresses will be > > dma_map'ed and > > passed to the device, allowing the NIC can perform DMA directly - the > > driver will not have to copy the buffer content into pre-allocated > > buffers/qpls (as in qpl mode). > > For ingress packets, copies are also eliminated as buffers are handed > > to > > the networking stack and then recycled or re-allocated as > > necessary, avoiding the use of skb_copy_to_linear_data(). > > > > This patch only introduces the option to the driver. > > Subsequent patches will add the ingress and egress functionality. > > > > Reviewed-by: Yangchun Fu <yangchun@google.com> > > Signed-off-by: Catherine Sullivan <csully@google.com> > > Signed-off-by: David Awogbemila <awogbemila@google.com> > > --- > > > ... > > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c > > b/drivers/net/ethernet/google/gve/gve_adminq.c > > index 24ae6a28a806..1e2d407cb9d2 100644 > > --- a/drivers/net/ethernet/google/gve/gve_adminq.c > > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c > > @@ -14,6 +14,57 @@ > > #define GVE_ADMINQ_SLEEP_LEN 20 > > #define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK 100 > > > > +#define GVE_DEVICE_OPTION_ERROR_FMT "%s option error:\n" \ > > +"Expected: length=%d, feature_mask=%x.\n" \ > > +"Actual: length=%d, feature_mask=%x.\n" > > + > > +static inline > > +struct gve_device_option *gve_get_next_option(struct > > > > Following Dave's policy, no static inline functions in C files. > This is control path so you don't really need the inline here. Okay, I'll move it to a header file. > > >
On Thu, Nov 19, 2020 at 04:22:24PM -0800, David Awogbemila wrote: > On Thu, Nov 19, 2020 at 12:21 PM Saeed Mahameed <saeed@kernel.org> wrote: > > > > On Wed, 2020-11-18 at 15:20 -0800, David Awogbemila wrote: > > > From: Catherine Sullivan <csully@google.com> > > > > > > Add support to describe device for parsing device options. As > > > the first device option, add raw addressing. > > > > > > "Raw Addressing" mode (as opposed to the current "qpl" mode) is an > > > operational mode which allows the driver avoid bounce buffer copies > > > which it currently performs using pre-allocated qpls > > > (queue_page_lists) > > > when sending and receiving packets. > > > For egress packets, the provided skb data addresses will be > > > dma_map'ed and > > > passed to the device, allowing the NIC can perform DMA directly - the > > > driver will not have to copy the buffer content into pre-allocated > > > buffers/qpls (as in qpl mode). > > > For ingress packets, copies are also eliminated as buffers are handed > > > to > > > the networking stack and then recycled or re-allocated as > > > necessary, avoiding the use of skb_copy_to_linear_data(). > > > > > > This patch only introduces the option to the driver. > > > Subsequent patches will add the ingress and egress functionality. > > > > > > Reviewed-by: Yangchun Fu <yangchun@google.com> > > > Signed-off-by: Catherine Sullivan <csully@google.com> > > > Signed-off-by: David Awogbemila <awogbemila@google.com> > > > --- > > > > > ... > > > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c > > > b/drivers/net/ethernet/google/gve/gve_adminq.c > > > index 24ae6a28a806..1e2d407cb9d2 100644 > > > --- a/drivers/net/ethernet/google/gve/gve_adminq.c > > > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c > > > @@ -14,6 +14,57 @@ > > > #define GVE_ADMINQ_SLEEP_LEN 20 > > > #define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK 100 > > > > > > +#define GVE_DEVICE_OPTION_ERROR_FMT "%s option error:\n" \ > > > +"Expected: length=%d, feature_mask=%x.\n" \ > > > +"Actual: length=%d, feature_mask=%x.\n" > > > + > > > +static inline > > > +struct gve_device_option *gve_get_next_option(struct > > > > > > > Following Dave's policy, no static inline functions in C files. > > This is control path so you don't really need the inline here. > > Okay, I'll move it to a header file. That's not what Saeed meant I suppose. Policy says that we let the compiler to make the decision whether or not such static function should be inlined. And since it's not a performance critical path as Saeed says then drop the inline and keep the rest as-is. > > > > > > >
On Thu, Nov 19, 2020 at 6:10 PM Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > On Thu, Nov 19, 2020 at 04:22:24PM -0800, David Awogbemila wrote: > > On Thu, Nov 19, 2020 at 12:21 PM Saeed Mahameed <saeed@kernel.org> wrote: > > > > > > On Wed, 2020-11-18 at 15:20 -0800, David Awogbemila wrote: > > > > From: Catherine Sullivan <csully@google.com> > > > > > > > > Add support to describe device for parsing device options. As > > > > the first device option, add raw addressing. > > > > > > > > "Raw Addressing" mode (as opposed to the current "qpl" mode) is an > > > > operational mode which allows the driver avoid bounce buffer copies > > > > which it currently performs using pre-allocated qpls > > > > (queue_page_lists) > > > > when sending and receiving packets. > > > > For egress packets, the provided skb data addresses will be > > > > dma_map'ed and > > > > passed to the device, allowing the NIC can perform DMA directly - the > > > > driver will not have to copy the buffer content into pre-allocated > > > > buffers/qpls (as in qpl mode). > > > > For ingress packets, copies are also eliminated as buffers are handed > > > > to > > > > the networking stack and then recycled or re-allocated as > > > > necessary, avoiding the use of skb_copy_to_linear_data(). > > > > > > > > This patch only introduces the option to the driver. > > > > Subsequent patches will add the ingress and egress functionality. > > > > > > > > Reviewed-by: Yangchun Fu <yangchun@google.com> > > > > Signed-off-by: Catherine Sullivan <csully@google.com> > > > > Signed-off-by: David Awogbemila <awogbemila@google.com> > > > > --- > > > > > > > ... > > > > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c > > > > b/drivers/net/ethernet/google/gve/gve_adminq.c > > > > index 24ae6a28a806..1e2d407cb9d2 100644 > > > > --- a/drivers/net/ethernet/google/gve/gve_adminq.c > > > > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c > > > > @@ -14,6 +14,57 @@ > > > > #define GVE_ADMINQ_SLEEP_LEN 20 > > > > #define GVE_MAX_ADMINQ_EVENT_COUNTER_CHECK 100 > > > > > > > > +#define GVE_DEVICE_OPTION_ERROR_FMT "%s option error:\n" \ > > > > +"Expected: length=%d, feature_mask=%x.\n" \ > > > > +"Actual: length=%d, feature_mask=%x.\n" > > > > + > > > > +static inline > > > > +struct gve_device_option *gve_get_next_option(struct > > > > > > > > > > Following Dave's policy, no static inline functions in C files. > > > This is control path so you don't really need the inline here. > > > > Okay, I'll move it to a header file. > > That's not what Saeed meant I suppose. Policy says that we let the > compiler to make the decision whether or not such static function should > be inlined. And since it's not a performance critical path as Saeed says > then drop the inline and keep the rest as-is. Oh I see, thanks for the clarification, I was going to remove the inline keyword and move it to a header file but I see now that I can just remove the inline keyword. thanks.
Patch 1: Use u8 instead of bool for raw_addressing bit in gve_priv structure. Simplify pointer arithmetic: use (option + 1) in gve_get_next_option. Separate option parsing switch statement into individual function. Patch 2: Use u8 instead of bool for raw_addressing bit in gve_gve_rx_data_queue structure. Correct typo in gve_desc.h comment (s/than/then/). Change gve_rx_data_slot from struct to union. Remove dma_mapping_error path change in gve_alloc_page - it should probably be a bug fix. Use & to obtain page address from data_ring->addr. Move declarations of local variables i and slots to if statement where they are used within gve_rx_unfill_pages. Simplify alloc_err path by using "while(i--)", eliminating need for extra "int j" variable in gve_prefill_rx_pages. Apply byteswap to constant in gve_rx_flip_buff. Remove gve_rx_raw_addressing as it does not do much more than gve_rx_add_frags. Remove stats update from elseif block, no need to optimize for infrequent case of work_done = 0. Patch 3: Use u8 instead of bool for can_flip in gve_rx_slot_page_info. Move comment in gve_rx_flip_buff to earlier, more relevant patch. Fix comment wrap in gve_rx_can_flip_buffers. Use ternary statement for gve_rx_can_flip_buffers. Correct comment in gve_rx_qpl. Patch 4: Use u8 instead of bool in gve_tx_ring structure. Get rid of unnecessary local variable "dma" in gve_dma_sync_for_device. Catherine Sullivan (3): gve: Add support for raw addressing device option gve: Add support for raw addressing to the rx path gve: Add support for raw addressing in the tx path David Awogbemila (1): gve: Rx Buffer Recycling drivers/net/ethernet/google/gve/gve.h | 38 +- drivers/net/ethernet/google/gve/gve_adminq.c | 90 ++++- drivers/net/ethernet/google/gve/gve_adminq.h | 15 +- drivers/net/ethernet/google/gve/gve_desc.h | 19 +- drivers/net/ethernet/google/gve/gve_main.c | 11 +- drivers/net/ethernet/google/gve/gve_rx.c | 403 ++++++++++++++----- drivers/net/ethernet/google/gve/gve_tx.c | 211 ++++++++-- 7 files changed, 620 insertions(+), 167 deletions(-)