Re: [PATCH libibverbs V4 2/5] Extend create_ah in order to pass L2 parameters to the provider

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 22/05/2014 11:52, Matan Barak wrote:
On 21/5/2014 10:55 PM, Jason Gunthorpe wrote:
On Sun, May 18, 2014 at 12:38:46PM +0300, Or Gerlitz wrote:
From: Matan Barak <matanb@xxxxxxxxxxxx>

In order to support IP based addressing, one needs to pass the L2
parameters to the provider drive. This is done through a new extendable
                                ^^^^
'driver'

Please provide a man page. In this case you probably want to provide a
patch to the existing man/ibv_create_ah.3 to discuss the 2nd entry
point and new fields.


Ok, I'll add this entry to the ibv_create_ah man page.

+enum ibv_ah_attr_ex_attr_mask {
+    IBV_AH_ATTR_EX_LL = 1UL << 0,
+    IBV_AH_ATTR_EX_VID = 1UL << 1,
+};

OK

+struct ibv_ah_attr_ex {
+    struct ibv_global_route    grh;
+    uint16_t        dlid;
+    uint8_t            sl;
+    uint8_t            src_path_bits;
+    uint8_t            static_rate;
+    uint8_t            is_global;
+    uint8_t            port_num;
+    uint32_t        comp_mask;

OK

+    union {
+        struct sockaddr        sa;
+        struct sockaddr_storage _s;
+    } ll;
+    uint16_t        vid;
+};

Hard to know exactly what is going on here without a man page, but I
thought one of the points was to provide an ethernet L2 MAC address?
Shouldn't there be a mechanism for that?

I see this:

+       attr_ex.ll.sa.sa_family = ARPHRD_ETHER;

Which makes no sense, sa_family should be an AF_* constant.


Kernel code *sometimes* use sa_family as ARPHRD_ETHER.

So, I think this bit needs some kind of work. If you want to setup
ethernet, then setup ethernet:

uint64_t eth_dmac
uint16_t eth_vid;

and what about all the trendy new ethernet stuff, overlay networks,
etc? Can that be expressed in there?


I don't want to make this Ethernet specific. That's why the previous pointer used a pointer. I don't mind creating a generic interface for link layer if the existing solutions aren't good enough. Any suggestions?

Hi Jason,

Can you please address Matan's comments? we'd like this thread to converge...

Or.


@@ -975,6 +998,8 @@ enum verbs_context_mask {

  struct verbs_context {
      /*  "grows up" - new fields go here */
+    struct ibv_ah * (*create_ah_ex)(struct ibv_pd *pd,
+                    struct ibv_ah_attr_ex *attr);

Can you check if 'attr' should be const? I see the existing API isn't
const, but I am suspecting that is a mistake?

You're probably right, but wouldn't we want to be aligned with the non-extended verb: struct ibv_ah * (*create_ah)(struct ibv_pd *pd, struct ibv_ah_attr *attr); Since the provider driver usually go through a common path for both the non-extended and the extended verb, this could cause an ugly const cast.


Jason


Matan


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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux