RE: [PATCH iproute2-next v3] rdma: Document IB device renaming option

[Ruhl, Michael J]

>-Original Message-
>From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
>ow...@vger.kernel.org] On Behalf Of Leon Romanovsky
>Sent: Sunday, November 4, 2018 2:11 PM
>To: David Ahern 
>Cc: Leon Romanovsky ; netdev
>; RDMA mailing list ;
>Stephen Hemminger 
>Subject: [PATCH iproute2-next v3] rdma: Document IB device renaming
>option
>
>From: Leon Romanovsky 

Hi Leon,

After looking at this and Steve Wise's changes for the ADDLINK/DELLINK,
it occurred to me that the driver that handed the name to ib_register_device()
might be interested in knowing that this name change occurred.

Are there plans to include a some kind of notify mechanism so drivers can
find out when things like this occur?

Is this something that should be done?

Thanks,

Mike

>[leonro@server /]$ lspci |grep -i Ether
>00:08.0 Ethernet controller: Red Hat, Inc. Virtio network device
>00:09.0 Ethernet controller: Mellanox Technologies MT27700 Family
>[ConnectX-4]
>[leonro@server /]$ sudo rdma dev
>1: mlx5_0: node_type ca fw 3.8. node_guid 5254:00c0:fe12:3455
>sys_image_guid 5254:00c0:fe12:3455
>[leonro@server /]$ sudo rdma dev set mlx5_0 name hfi1_0
>[leonro@server /]$ sudo rdma dev
>1: hfi1_0: node_type ca fw 3.8. node_guid 5254:00c0:fe12:3455
>sys_image_guid 5254:00c0:fe12:3455
>
>Signed-off-by: Leon Romanovsky 
>---
>Changelog:
>v2->v3:
> * Dropped "to be named" words from example section of man
>---
> man/man8/rdma-dev.8 | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/man/man8/rdma-dev.8 b/man/man8/rdma-dev.8
>index 461681b6..7c275180 100644
>--- a/man/man8/rdma-dev.8
>+++ b/man/man8/rdma-dev.8
>@@ -1,6 +1,6 @@
> .TH RDMA\-DEV 8 "06 Jul 2017" "iproute2" "Linux"
> .SH NAME
>-rdmak-dev \- RDMA device configuration
>+rdma-dev \- RDMA device configuration
> .SH SYNOPSIS
> .sp
> .ad l
>@@ -22,10 +22,18 @@ rdmak-dev \- RDMA device configuration
> .B rdma dev show
> .RI "[ " DEV " ]"
>
>+.ti -8
>+.B rdma dev set
>+.RI "[ " DEV " ]"
>+.BR name
>+.BR NEWNAME
>+
> .ti -8
> .B rdma dev help
>
> .SH "DESCRIPTION"
>+.SS rdma dev set - rename rdma device
>+
> .SS rdma dev show - display rdma device attributes
>
> .PP
>@@ -45,6 +53,11 @@ rdma dev show mlx5_3
> Shows the state of specified RDMA device.
> .RE
> .PP
>+rdma dev set mlx5_3 name rdma_0
>+.RS 4
>+Renames the mlx5_3 device to rdma_0.
>+.RE
>+.PP
>
> .SH SEE ALSO
> .BR rdma (8),
>--
>2.19.1

RE: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow counters

[Ruhl, Michael J]

>-Original Message-
>From: Leon Romanovsky [mailto:l...@kernel.org]
>Sent: Thursday, May 31, 2018 9:44 AM
>To: Doug Ledford ; Jason Gunthorpe
>
>Cc: Leon Romanovsky ; RDMA mailing list r...@vger.kernel.org>; Boris Pismenny ; Matan
>Barak ; Ruhl, Michael J ;
>Or Gerlitz ; Raed Salem ;
>Yishai Hadas ; Saeed Mahameed
>; linux-netdev 
>Subject: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow
>counters
>
>From: Raed Salem 
>
>The struct ib_uverbs_flow_spec_action_count associates
>a counters object with the flow.
>
>Post this association the flow counters can be read via
>the counters object.
>
>Reviewed-by: Yishai Hadas 
>Signed-off-by: Raed Salem 
>Signed-off-by: Leon Romanovsky 
>---
> drivers/infiniband/core/uverbs.h |  1 +
> drivers/infiniband/core/uverbs_cmd.c | 81
>+++-
> include/uapi/rdma/ib_user_verbs.h| 13 ++
> 3 files changed, 84 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/infiniband/core/uverbs.h
>b/drivers/infiniband/core/uverbs.h
>index 5b2461fa634d..c0d40fc3a53a 100644
>--- a/drivers/infiniband/core/uverbs.h
>+++ b/drivers/infiniband/core/uverbs.h
>@@ -263,6 +263,7 @@ struct ib_uverbs_flow_spec {
>   struct ib_uverbs_flow_spec_action_tag   flow_tag;
>   struct ib_uverbs_flow_spec_action_drop  drop;
>   struct ib_uverbs_flow_spec_action_handle action;
>+  struct ib_uverbs_flow_spec_action_count flow_count;
>   };
> };
>
>diff --git a/drivers/infiniband/core/uverbs_cmd.c
>b/drivers/infiniband/core/uverbs_cmd.c
>index ddb9d79691be..3179a95c6f5e 100644
>--- a/drivers/infiniband/core/uverbs_cmd.c
>+++ b/drivers/infiniband/core/uverbs_cmd.c
>@@ -2748,43 +2748,82 @@ ssize_t ib_uverbs_detach_mcast(struct
>ib_uverbs_file *file,
> struct ib_uflow_resources {
>   size_t  max;
>   size_t  num;
>-  struct ib_flow_action   *collection[0];
>+  size_t  collection_num;
>+  size_t  counters_num;
>+  struct ib_counters  **counters;
>+  struct ib_flow_action   **collection;
> };
>
> static struct ib_uflow_resources *flow_resources_alloc(size_t num_specs)
> {
>   struct ib_uflow_resources *resources;
>
>-  resources =
>-  kmalloc(sizeof(*resources) +
>-  num_specs * sizeof(*resources->collection),
>GFP_KERNEL);
>+  resources = kzalloc(sizeof(*resources), GFP_KERNEL);
>
>   if (!resources)
>-  return NULL;
>+  goto err_res;

Why the new goto?

>+
>+  resources->counters =
>+  kcalloc(num_specs, sizeof(*resources->counters),
>GFP_KERNEL);
>+
>+  if (!resources->counters)
>+  goto err_cnt;

kcalloc() zeros stuff.  Could you just have a single common goto for the
cleanup?

Mike

>+
>+  resources->collection =
>+  kcalloc(num_specs, sizeof(*resources->collection),
>GFP_KERNEL);
>+
>+  if (!resources->collection)
>+  goto err_collection;
>
>-  resources->num = 0;
>   resources->max = num_specs;
>
>   return resources;
>+
>+err_collection:
>+  kfree(resources->counters);
>+err_cnt:
>+  kfree(resources);
>+err_res:
>+  return NULL;
> }
>
> void ib_uverbs_flow_resources_free(struct ib_uflow_resources *uflow_res)
> {
>   unsigned int i;
>
>-  for (i = 0; i < uflow_res->num; i++)
>+  for (i = 0; i < uflow_res->collection_num; i++)
>   atomic_dec(_res->collection[i]->usecnt);
>
>+  for (i = 0; i < uflow_res->counters_num; i++)
>+  atomic_dec(_res->counters[i]->usecnt);
>+
>+  kfree(uflow_res->collection);
>+  kfree(uflow_res->counters);
>   kfree(uflow_res);
> }
>
> static void flow_resources_add(struct ib_uflow_resources *uflow_res,
>- struct ib_flow_action *action)
>+ enum ib_flow_spec_type type,
>+ void *ibobj)
> {
>   WARN_ON(uflow_res->num >= uflow_res->max);
>
>-  atomic_inc(>usecnt);
>-  uflow_res->collection[uflow_res->num++] = action;
>+  switch (type) {
>+  case IB_FLOW_SPEC_ACTION_HANDLE:
>+  atomic_inc(&((struct ib_flow_action *)ibobj)->usecnt);
>+  uflow_res->collection[uflow_res->collection_num++] =
>+  (struct ib_flow_action *)ibobj;
>+  break;
>+  case IB_FLOW_SPEC_ACTION_COUNT:
>+  atomic_inc(&((s

RE: [PATCH rdma-next v3 08/14] IB/core: Support passing uhw for create_flow

[Ruhl, Michael J]

>-Original Message-
>From: Leon Romanovsky [mailto:l...@kernel.org]
>Sent: Thursday, May 31, 2018 9:44 AM
>To: Doug Ledford ; Jason Gunthorpe
>
>Cc: Leon Romanovsky ; RDMA mailing list r...@vger.kernel.org>; Boris Pismenny ; Matan
>Barak ; Ruhl, Michael J ;
>Or Gerlitz ; Raed Salem ;
>Yishai Hadas ; Saeed Mahameed
>; linux-netdev 
>Subject: [PATCH rdma-next v3 08/14] IB/core: Support passing uhw for
>create_flow
>
>From: Matan Barak 
>
>This is required when user-space drivers need to pass extra information
>regarding how to handle this flow steering specification.
>
>Reviewed-by: Yishai Hadas 
>Signed-off-by: Matan Barak 
>Signed-off-by: Boris Pismenny 
>Signed-off-by: Leon Romanovsky 
>---
> drivers/infiniband/core/uverbs_cmd.c | 7 ++-
> drivers/infiniband/core/verbs.c  | 2 +-
> drivers/infiniband/hw/mlx4/main.c| 6 +-
> drivers/infiniband/hw/mlx5/main.c| 7 ++-
> include/rdma/ib_verbs.h  | 3 ++-
> 5 files changed, 20 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/infiniband/core/uverbs_cmd.c
>b/drivers/infiniband/core/uverbs_cmd.c
>index e74262ee104c..ddb9d79691be 100644
>--- a/drivers/infiniband/core/uverbs_cmd.c
>+++ b/drivers/infiniband/core/uverbs_cmd.c
>@@ -3542,11 +3542,16 @@ int ib_uverbs_ex_create_flow(struct
>ib_uverbs_file *file,
>   err = -EINVAL;
>   goto err_free;
>   }
>-  flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);
>+
>+  flow_id = qp->device->create_flow(qp, flow_attr,
>+IB_FLOW_DOMAIN_USER, uhw);
>+

If the create_flow() callback is not defined, won't this cause a problem?

ib_flow_create() checks for the NULL.

Mike


>   if (IS_ERR(flow_id)) {
>   err = PTR_ERR(flow_id);
>   goto err_free;
>   }
>+  atomic_inc(>usecnt);
>+  flow_id->qp = qp;
>   flow_id->uobject = uobj;
>   uobj->object = flow_id;
>   uflow = container_of(uobj, typeof(*uflow), uobject);
>diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>index 6ddfb1fade79..0b56828c1319 100644
>--- a/drivers/infiniband/core/verbs.c
>+++ b/drivers/infiniband/core/verbs.c
>@@ -1983,7 +1983,7 @@ struct ib_flow *ib_create_flow(struct ib_qp *qp,
>   if (!qp->device->create_flow)
>   return ERR_PTR(-EOPNOTSUPP);
>
>-  flow_id = qp->device->create_flow(qp, flow_attr, domain);
>+  flow_id = qp->device->create_flow(qp, flow_attr, domain, NULL);
>   if (!IS_ERR(flow_id)) {
>   atomic_inc(>usecnt);
>   flow_id->qp = qp;
>diff --git a/drivers/infiniband/hw/mlx4/main.c
>b/drivers/infiniband/hw/mlx4/main.c
>index bf12394c13c1..6fe5d5d1d1d9 100644
>--- a/drivers/infiniband/hw/mlx4/main.c
>+++ b/drivers/infiniband/hw/mlx4/main.c
>@@ -1848,7 +1848,7 @@ static int mlx4_ib_add_dont_trap_rule(struct
>mlx4_dev *dev,
>
> static struct ib_flow *mlx4_ib_create_flow(struct ib_qp *qp,
>   struct ib_flow_attr *flow_attr,
>-  int domain)
>+  int domain, struct ib_udata *udata)
> {
>   int err = 0, i = 0, j = 0;
>   struct mlx4_ib_flow *mflow;
>@@ -1866,6 +1866,10 @@ static struct ib_flow *mlx4_ib_create_flow(struct
>ib_qp *qp,
>   (flow_attr->type != IB_FLOW_ATTR_NORMAL))
>   return ERR_PTR(-EOPNOTSUPP);
>
>+  if (udata &&
>+  udata->inlen && !ib_is_udata_cleared(udata, 0, udata->inlen))
>+  return ERR_PTR(-EOPNOTSUPP);
>+
>   memset(type, 0, sizeof(type));
>
>   mflow = kzalloc(sizeof(*mflow), GFP_KERNEL);
>diff --git a/drivers/infiniband/hw/mlx5/main.c
>b/drivers/infiniband/hw/mlx5/main.c
>index 92879d2d3026..fb31a719ee25 100644
>--- a/drivers/infiniband/hw/mlx5/main.c
>+++ b/drivers/infiniband/hw/mlx5/main.c
>@@ -3371,7 +3371,8 @@ static struct mlx5_ib_flow_handler
>*create_sniffer_rule(struct mlx5_ib_dev *dev,
>
> static struct ib_flow *mlx5_ib_create_flow(struct ib_qp *qp,
>  struct ib_flow_attr *flow_attr,
>- int domain)
>+ int domain,
>+ struct ib_udata *udata)
> {
>   struct mlx5_ib_dev *dev = to_mdev(qp->device);
>   struct mlx5_ib_qp *mqp = to_mqp(qp);
>@@ -3383,6 +3384,10 @@ static struct ib_flow *mlx5_ib_create_flow(struct
>ib_qp *qp,
>   int err;
>   int underlay_qpn;
>
>+  if (udata &&
>+  udata->

RE: [PATCH rdma-next v2 01/13] IB/uverbs: Add an ib_uobject getter to ioctl() infrastructure

[Ruhl, Michael J]

>-Original Message-
>From: Jason Gunthorpe [mailto:j...@mellanox.com]
>Sent: Tuesday, May 29, 2018 4:21 PM
>To: Ruhl, Michael J 
>Cc: Leon Romanovsky ; Doug Ledford
>; Leon Romanovsky ; RDMA
>mailing list ; Boris Pismenny
>; Matan Barak ; Raed
>Salem ; Yishai Hadas ; Saeed
>Mahameed ; linux-netdev
>
>Subject: Re: [PATCH rdma-next v2 01/13] IB/uverbs: Add an ib_uobject getter
>to ioctl() infrastructure
>
>On Tue, May 29, 2018 at 07:31:22PM +, Ruhl, Michael J wrote:
>> >-   struct ib_uverbs_destroy_cq_resp resp;
>> >struct ib_uobject *uobj =
>> >-   uverbs_attr_get(attrs,
>> >UVERBS_ATTR_DESTROY_CQ_HANDLE)->obj_attr.uobject;
>> >-   struct ib_ucq_object *obj = container_of(uobj, struct ib_ucq_object,
>> >-uobject);
>> >+   uverbs_attr_get_uobject(attrs,
>> >UVERBS_ATTR_DESTROY_CQ_HANDLE);
>> >+   struct ib_uverbs_destroy_cq_resp resp;
>> >+   struct ib_ucq_object *obj;
>> >int ret;
>> >
>> >+   if (IS_ERR(uobj))
>> >+   return PTR_ERR(uobj);
>> >+
>>
>> I remember a conversation that if an method attribute was mandatory, that
>you did not need to
>> test the uobj for error (since it was checked in the infrastructure).
>
>Yes.
>
>> Is this error check necessary?
>
>No
>
>But there is no way to check one way or the other at compile time
>right now, and omitting the check makes smatch mad.

Is smatch going to get mad at (same patch):

diff --git a/drivers/infiniband/core/uverbs_std_types_flow_action.c 
b/drivers/infiniband/core/uverbs_std_types_flow_action.c
index b4f016dfa23d..a7be51cf2e42 100644
--- a/drivers/infiniband/core/uverbs_std_types_flow_action.c
+++ b/drivers/infiniband/core/uverbs_std_types_flow_action.c
@@ -320,7 +320,7 @@ static int 
UVERBS_HANDLER(UVERBS_METHOD_FLOW_ACTION_ESP_CREATE)(struct ib_device
return ret;

/* No need to check as this attribute is marked as MANDATORY */
-   uobj = uverbs_attr_get(attrs, 
UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE)->obj_attr.uobject;
+   uobj = uverbs_attr_get_uobject(attrs, 
UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE);
action = ib_dev->create_flow_action_esp(ib_dev, _attr.hdr, attrs);
if (IS_ERR(action))
return PTR_ERR(action);
@@ -350,7 +350,7 @@ static int 
UVERBS_HANDLER(UVERBS_METHOD_FLOW_ACTION_ESP_MODIFY)(struct ib_device
if (ret)
return ret;

-   uobj = uverbs_attr_get(attrs, 
UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE)->obj_attr.uobject;
+   uobj = uverbs_attr_get_uobject(attrs, 
UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE);
action = uobj->object;

?

If not,

Reviewed-by: Michael J. Ruhl 

Thanks,

Mike

>We need some more patches to be able to safely omit the check...
>
>Jason

RE: [PATCH rdma-next v2 01/13] IB/uverbs: Add an ib_uobject getter to ioctl() infrastructure

[Ruhl, Michael J]

>-Original Message-
>From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
>ow...@vger.kernel.org] On Behalf Of Leon Romanovsky
>Sent: Tuesday, May 29, 2018 9:09 AM
>To: Doug Ledford ; Jason Gunthorpe
>
>Cc: Leon Romanovsky ; RDMA mailing list r...@vger.kernel.org>; Boris Pismenny ; Matan
>Barak ; Raed Salem ; Yishai
>Hadas ; Saeed Mahameed
>; linux-netdev 
>Subject: [PATCH rdma-next v2 01/13] IB/uverbs: Add an ib_uobject getter to
>ioctl() infrastructure
>
>From: Matan Barak 
>
>Previously, the user had to dig inside the attribute to get the uobject.
>Add a helper function that correctly extract it (and do the required
>checks) for him/her.
>
>Tested-by: Michael Guralnik 
>Signed-off-by: Matan Barak 
>Signed-off-by: Leon Romanovsky 
>---
> drivers/infiniband/core/uverbs_std_types_cq.c  | 23 +++-
>--
> .../infiniband/core/uverbs_std_types_flow_action.c |  4 ++--
> include/rdma/uverbs_ioctl.h| 11 +++
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c
>b/drivers/infiniband/core/uverbs_std_types_cq.c
>index b0dbae9dd0d7..3d293d01afea 100644
>--- a/drivers/infiniband/core/uverbs_std_types_cq.c
>+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
>@@ -65,7 +65,6 @@ static int
>UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev,
>   struct ib_cq_init_attr attr = {};
>   struct ib_cq   *cq;
>   struct ib_uverbs_completion_event_file*ev_file = NULL;
>-  const struct uverbs_attr *ev_file_attr;
>   struct ib_uobject *ev_file_uobj;
>
>   if (!(ib_dev->uverbs_cmd_mask & 1ULL <<
>IB_USER_VERBS_CMD_CREATE_CQ))
>@@ -87,10 +86,8 @@ static int
>UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev,
>
>   UVERBS_ATTR_CREATE_CQ_FLAGS)))
>   return -EFAULT;
>
>-  ev_file_attr = uverbs_attr_get(attrs,
>UVERBS_ATTR_CREATE_CQ_COMP_CHANNEL);
>-  if (!IS_ERR(ev_file_attr)) {
>-  ev_file_uobj = ev_file_attr->obj_attr.uobject;
>-
>+  ev_file_uobj = uverbs_attr_get_uobject(attrs,
>UVERBS_ATTR_CREATE_CQ_COMP_CHANNEL);
>+  if (!IS_ERR(ev_file_uobj)) {
>   ev_file = container_of(ev_file_uobj,
>  struct ib_uverbs_completion_event_file,
>  uobj_file.uobj);
>@@ -102,8 +99,8 @@ static int
>UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev,
>   goto err_event_file;
>   }
>
>-  obj = container_of(uverbs_attr_get(attrs,
>-
>UVERBS_ATTR_CREATE_CQ_HANDLE)->obj_attr.uobject,

See comment below on error checking.  Does this need the error check?

>+  obj = container_of(uverbs_attr_get_uobject(attrs,
>+
>UVERBS_ATTR_CREATE_CQ_HANDLE),
>  typeof(*obj), uobject);
>   obj->uverbs_file   = ucontext->ufile;
>   obj->comp_events_reported  = 0;
>@@ -170,13 +167,17 @@ static int
>UVERBS_HANDLER(UVERBS_METHOD_CQ_DESTROY)(struct ib_device
>*ib_dev,
>   struct ib_uverbs_file *file,
>   struct uverbs_attr_bundle
>*attrs)
> {
>-  struct ib_uverbs_destroy_cq_resp resp;
>   struct ib_uobject *uobj =
>-  uverbs_attr_get(attrs,
>UVERBS_ATTR_DESTROY_CQ_HANDLE)->obj_attr.uobject;
>-  struct ib_ucq_object *obj = container_of(uobj, struct ib_ucq_object,
>-   uobject);
>+  uverbs_attr_get_uobject(attrs,
>UVERBS_ATTR_DESTROY_CQ_HANDLE);
>+  struct ib_uverbs_destroy_cq_resp resp;
>+  struct ib_ucq_object *obj;
>   int ret;
>
>+  if (IS_ERR(uobj))
>+  return PTR_ERR(uobj);
>+

I remember a conversation that if an method attribute was mandatory, that you 
did not need to
test the uobj for error (since it was checked in the infrastructure).

Is this error check necessary?

Thanks

Mike

>+  obj = container_of(uobj, struct ib_ucq_object, uobject);
>+
>   if (!(ib_dev->uverbs_cmd_mask & 1ULL <<
>IB_USER_VERBS_CMD_DESTROY_CQ))
>   return -EOPNOTSUPP;
>
>diff --git a/drivers/infiniband/core/uverbs_std_types_flow_action.c
>b/drivers/infiniband/core/uverbs_std_types_flow_action.c
>index b4f016dfa23d..a7be51cf2e42 100644
>--- a/drivers/infiniband/core/uverbs_std_types_flow_action.c
>+++ b/drivers/infiniband/core/uverbs_std_types_flow_action.c
>@@ -320,7 +320,7 @@ static int
>UVERBS_HANDLER(UVERBS_METHOD_FLOW_ACTION_ESP_CREATE)(struct
>ib_device
>   return ret;
>
>   /* No need to check as this attribute is marked as MANDATORY */
>-  uobj = uverbs_attr_get(attrs,
>UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE)->obj_attr.uobject;
>+  uobj = uverbs_attr_get_uobject(attrs,
>UVERBS_ATTR_FLOW_ACTION_ESP_HANDLE);
>   action = ib_dev->create_flow_action_esp(ib_dev, _attr.hdr,
>attrs);
>   if (IS_ERR(action))
>   return PTR_ERR(action);
>@@ 

RE: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in mlx4_en_select_queue

[Ruhl, Michael J]

>-Original Message-
>From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
>ow...@vger.kernel.org] On Behalf Of Alexander Duyck
>Sent: Friday, April 27, 2018 2:07 PM
>To: netdev@vger.kernel.org; da...@davemloft.net
>Cc: linux-r...@vger.kernel.org; Dalessandro, Dennis
>; Vishwanathapura, Niranjana
>; tar...@mellanox.com
>Subject: [PATCH 2/3] mlx4: Don't bother using skb_tx_hash in
>mlx4_en_select_queue
>
>The code in the fallback path has supported XDP in conjunction with the Tx
>traffic classification for TCs for over a year now. So instead of just
>calling skb_tx_hash for every packet we are better off using the fallback
>since that will record the Tx queue to the socket and then that can be used
>instead of having to recompute the hash every time.
>
>Signed-off-by: Alexander Duyck 
>---
> drivers/net/ethernet/mellanox/mlx4/en_tx.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>index 6b68537..0227786 100644
>--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>@@ -694,7 +694,7 @@ u16 mlx4_en_select_queue(struct net_device *dev,
>struct sk_buff *skb,
>   u16 rings_p_up = priv->num_tx_rings_p_up;
>
>   if (netdev_get_num_tc(dev))
>-  return skb_tx_hash(dev, skb);
>+  return fallback(dev, skb);
>
>   return fallback(dev, skb) % rings_p_up;

Hi Alexander,

The final return fallback() call is doing a % rings_p_up.

Do you need to do that for the new fallback() call?

Maybe you can get rid of the netdev_get_num_tc() call altogether?

Thanks,

Mike

> }
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html